Conversation
| } | ||
| } | ||
|
|
||
| function getResizedPng(xName, yName, original, newSize) { |
There was a problem hiding this comment.
Will this eventually need to be used on its own, or was it pulled out for readability?
There was a problem hiding this comment.
Yes, since the resizing/repositioning logic is being recycled this will later be used to generate the resized PNGs for the best fit position on resized alt-tests.
There was a problem hiding this comment.
Right right, we were going to assume the best position is the same for all and just skip straight here for the alts
|
|
||
| const screenshotOpts = { | ||
| animations: 'disabled', | ||
| path: updateGoldens ? goldenFileName : screenshotFileName |
There was a problem hiding this comment.
Assuming this got pulled into the takeScreenshot below because the path will now change each time we take the screenshot (to add the .alt part).
|
Neither the story nor the PR description explains what this is doing/solving. Can we add some details somewhere? |
| return await page.screenshot({ path, ...screenshotOpts }); | ||
| } | ||
|
|
||
| const screenshotFileBuffer = await takeScreenshot(); |
There was a problem hiding this comment.
I assume we'll take all the screenshots here, then repeatedly run runCompareTest to iterate over them all? The "no golden exists" scenario makes this confusing... what if you have some goldens for a specific test, but not all of them? And we can only go out to the "resize" function once 🤔
Basically, do we try to run everything and give back a big combined error message? Or fail at the first sign of a problem, but give no additional info about the others? I think it'll need to be the first one, so I guess we'll have to modify the below to give back a similar structure to discussed in the last PR. Something like:
{
default: { pass: false, message: 'No golden exists. Use the "--golden" CLI flag to re-run and re-generate goldens.' },
dark: { resizeRequired: true },
rtl: { pass: true }
}
And then return all of that, have brightspace-visual-diff-compare-resize command handle the ones that say resizeRequired: true, then format all of that into a nice error message if any of them are pass: false.
There was a problem hiding this comment.
Maybe the TestInfoManager can help and store the error info as well
There was a problem hiding this comment.
The idea was to run everything and fail separately so a combined error message is returned based on the error messages of all tests(e.g. "One or more tests failed...", "All tests passed"). Then maybe list them all like (alt) Error message
Also, the test info map was already able to understand the type of error:
- Was there a test entry without a
goldenproperty? Missing golden - Is there a
byteSizeproperty andpixelDiffis 0? No diff but different size - Is there a
pixelDiff? Screenshots are different - Neither prop is set? It passed
I'm not entirely sure for RTL but for color modes, I don't think we expect the sizing of things to change, so resizing logic is recycled, meaning one fails then they all fail and pass through the -resize logic.
There was a problem hiding this comment.
Sounds good, I think we'll just have to rearrange the logic a bit for cases where say, the default is missing a golden but dark has one and needs resizing, or vice versa. Since right now, that's checked before moving to the resize command.
There was a problem hiding this comment.
Right, I think we can have a guard for resizes(i.e. check for a golden and set it on the test info but return right after if some other test had to resize) and then check for existing golden(i.e. getTestInfo has a golden key) on the resize testing
There was a problem hiding this comment.
Another potential... we just, always set this.test.timeout(0);? Remove the need for two commands?
I did this so long ago, I can't remember if there were any risks to having it set before taking the screenshots. I think I was just being cautious. It expands the scope of what might cause it to hang from this to also the screenshot-taking piece.
|
One thing we didn't really discuss with this overall strategy is that, if the test fails spectacularly (like an issue causes the test to timeout/crash), you're gonna get weird behavior. All alt goldens being deleted that didn't get to have a screenshot taken, missing |
Thanks, added some context to both |
|
🎉 This PR is included in version 1.38.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
GAUD-9495
To be able to test incoming dark mode, it was proposed to be able to define alternative tests. This means conditionally(via golden/ describe options) re-running certain tests against a predefined different setup(e.g. dark mode, rtl, etc.).
To be able to do add these tests the vdiff library must be refactored to handle such tests, this PR allows the ability to make tests re-runnable