Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding configCampaignEvents.js and first test #162

Merged
merged 8 commits into from
Feb 3, 2023

Conversation

vaughnwalters
Copy link
Collaborator

Edited a few files that needed to change to be able to add configCampaignEvents.js and then wrote first test in configCampaignEvents.js as proof that it is working

@nicholasray nicholasray self-requested a review January 27, 2023 23:52
Comment on lines 19 to 40
const scenarios = tests.map( ( test ) => {
// On mobile, short pages will cause false positives on some patchsets e.g.
// the switchover to grid CSS. To avoid this use the viewport selector when
// not defined.
const urlParts = test.path.split( '?' );
const urlPath = urlParts[ 0 ];
const urlQueryString = urlParts[ 1 ] || '';
const useViewportSelector = [
'/wiki/Special:EnableEventRegistration'
].includes( urlPath ) || (
[
'action=history'
].filter( ( t ) => urlQueryString.includes( t ) )
).length > 0;

return Object.assign( {
selectors: useViewportSelector ? [ 'viewport' ] : undefined
}, test, {
url: `${BASE_URL}${test.path}`,
misMatchThreshold: 0.04
}, test );
} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may be able to replace this with the following:

Suggested change
const scenarios = tests.map( ( test ) => {
// On mobile, short pages will cause false positives on some patchsets e.g.
// the switchover to grid CSS. To avoid this use the viewport selector when
// not defined.
const urlParts = test.path.split( '?' );
const urlPath = urlParts[ 0 ];
const urlQueryString = urlParts[ 1 ] || '';
const useViewportSelector = [
'/wiki/Special:EnableEventRegistration'
].includes( urlPath ) || (
[
'action=history'
].filter( ( t ) => urlQueryString.includes( t ) )
).length > 0;
return Object.assign( {
selectors: useViewportSelector ? [ 'viewport' ] : undefined
}, test, {
url: `${BASE_URL}${test.path}`,
misMatchThreshold: 0.04
}, test );
} );
const scenarios = tests.map( ( test ) => {
return {
url: `${BASE_URL}${test.path}`,
misMatchThreshold: 0.04,
selectors: [ 'main' ],
removeSelectors: [ '.vector-column-end' ],
...test
};
} );

Note that by using the selectors option, we're only taking a screenshot of the main element, and we're removing the page tools menu with the removeSelectors option. If you capture the whole viewport, you might get more noise than you want, as the web team frequently makes changes the the UI around the content and that might flag failures that are not relevant to this form. It's up to you though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Okay yes, this makes sense thank you! I have made this change. Also, do you find that misMatchThreshold: 0.04 is generally a pretty safe place to keep it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! For the desktop report, 0.04% has worked well for us so far, but YMMV. You can check the actual diff % of each test result by hovering over the test name. If you're consistently noticing false positives, you may need to bump up the misMatchThreshold to match the diff % you're noticing. However, I've found that nearly all of our passing tests actually achieve a diff % of 0 (meaning all pixels match) unless they are flaky.

diff %

@nicholasray
Copy link
Contributor

👍 Great work @vaughnwalters !

@nicholasray nicholasray merged commit d933e1b into wikimedia:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants