@bx80 opened this Pull Request on November 6th 2022 Contributor

Description:

Checks for GA3, GA4 or GTM usage on a site when showing the tracking code / getting started screen and if present, will show a link to the appropriate guide on how to migrate to Matomo.

Ref: DEV-3005

Review

@bx80 commented on November 9th 2022 Contributor

The installation test was failing because the site content detection values required by the instructions email and tracking code templates were not being populated for fresh installs. I've updated the installation controller to do a site content detection request as part of the install wizard and provide the required values. I've also added a new fixture which will mock the site content detector finding nothing on the site and added this to the installation UI test so that the test doesn't make live requests to the test site.

@peterhashair commented on November 9th 2022 Contributor

@bx80 installation works now, once the tests are passed, will do another review :)

@bx80 commented on November 9th 2022 Contributor

@peterhashair If you mean the data in the getConsentManagerDefinitions() method, that's not test data, it's the data used to detect the known consent managers and their guide URLs. This PR is branched from #19821, which is where that code is used. We could move the definitions to a set of external files, but I can't see much benefit to this.

@peterhashair commented on November 10th 2022 Contributor

@bx80 sorry I mean, those functions private function checkForTestData() : bool below that as well.

@bx80 commented on November 10th 2022 Contributor

@peterhashair The site content detector gets called as part of the UI tests and while it's possible to mock class methods using PHPUnit for unit tests, I haven't seen a good way to do this with UI tests which will interact with the class method by way of an API call. The nearest existing pattern I found in the codebase was the location provider where a mock location provider class is set as the default provider via an option setting. The approach here is broadly similar, where the test fixture can set test data as an option and the class will check for it and return the test data values if they exist. Not the most elegant solution, but it works and is flexible. I'm open to better ideas though,

@sgiehl Do you have a recommended approach for this scenario?

@sgiehl commented on November 10th 2022 Member

@sgiehl Do you have a recommended approach for this scenario?

I guess we have this solved in various ways. I haven't had a look at the detailed solution here. But I guess I would have tried to somehow solve it using DI. So the class responsible for the detection is loaded using DI. And for ui tests, you could then overwrite the used class with some sort of test class, that returns fake response.

@bx80 commented on November 11th 2022 Contributor

Thanks @sgiehl, I've reworked the UI tests to use DI :+1:

This Pull Request was closed on November 13th 2022
Powered by GitHub Issue Mirror