-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
configCampaignEvents.js
Outdated
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 ); | ||
} ); |
There was a problem hiding this comment.
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:
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
👍 Great work @vaughnwalters ! |
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