Skip to content

fix: make tests re-runnable #939

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.

Sign up for GitHub

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

Merged
GZolla merged 4 commits into from
Apr 1, 2026
Merged

fix: make tests re-runnable #939

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ed933e
fix: make tests re-runnable
GZolla Mar 31, 2026
f183200
Appease Linter
GZolla Mar 31, 2026
9158abc
From review
GZolla Mar 31, 2026
5f7cd61
From review 2
GZolla Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 103 additions & 88 deletions src/server/visual-diff-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,31 +51,46 @@ async function clearDiffPaths(dir) {
}
}

function getResizedPng(xName, yName, original, newSize) {
Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026

Choose a reason for hiding this comment

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

Will this eventually need to be used on its own, or was it pulled out for readability?

Copy link
Copy Markdown
Contributor Author

@GZolla GZolla Mar 31, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026

Choose a reason for hiding this comment

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

Right right, we were going to assume the best position is the same for all and just skip straight here for the alts

const xOffsets = {
left: 0,
center: Math.floor((newSize.width - original.width) / 2),
right: newSize.width - original.width
};
const yOffsets = {
top: 0,
center: Math.floor((newSize.height - original.height) / 2),
bottom: newSize.height - original.height
};
const resized = new PNG(newSize);
PNG.bitblt(original, resized, 0, 0, original.width, original.height, xOffsets[xName], yOffsets[yName]);
return resized;
}

async function createComparisonPNGs(original, newSize) {
const resizedPNGs = [];
[
{ name: 'top', coord: 0 },
{ name: 'center', coord: Math.floor((newSize.height - original.height) / 2) },
{ name: 'bottom', coord: newSize.height - original.height }
].forEach(y => {
[
{ name: 'left', coord: 0 },
{ name: 'center', coord: Math.floor((newSize.width - original.width) / 2) },
{ name: 'right', coord: newSize.width - original.width }
].forEach(x => { // TODO: position added for reports, remove/adjust as needed
[ 'top', 'center', 'bottom' ].forEach(y => {
['left', 'center', 'right'].forEach(x => {
if (original.width === newSize.width && original.height === newSize.height) {
resizedPNGs.push({ png: original, position: `${y.name}-${x.name}` });
resizedPNGs.push({ png: original, position: `${y}-${x}` });
} else {
const resized = new PNG(newSize);
PNG.bitblt(original, resized, 0, 0, original.width, original.height, x.coord, y.coord);
resizedPNGs.push({ png: resized, position: `${y.name}-${x.name}` });
const resized = getResizedPng(x, y, original, newSize);
resizedPNGs.push({ png: resized, position: `${y}-${x}` });
}
});
});

return resizedPNGs;
}

function getPixelsDiff(screenshotImage, goldenImage) {
const diff = new PNG({ width: screenshotImage.width, height: screenshotImage.height });
const pixelsDiff = pixelmatch(
screenshotImage.data, goldenImage.data, diff.data, screenshotImage.width, screenshotImage.height, { diffMask: true, threshold: DEFAULT_TOLERANCE }
);
return { diff, pixelsDiff };
}

async function tryMoveFile(srcFileName, destFileName) {
await mkdir(dirname(destFileName), { recursive: true });
try {
Expand Down Expand Up @@ -151,89 +166,92 @@ export function visualDiff({ updateGoldens = false, runSubset = false } = {}) {
}

const screenshotOpts = {
animations: 'disabled',
path: updateGoldens ? goldenFileName : screenshotFileName
Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

@GZolla GZolla Mar 31, 2026

Choose a reason for hiding this comment

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

Correct

animations: 'disabled'
};

if (payload.fullPage) screenshotOpts.fullPage = true;
if (payload.rect) screenshotOpts.clip = payload.rect;

const page = session.browser.getPage(session.id);
await page.screenshot(screenshotOpts);
async function takeScreenshot() {
const path = updateGoldens ? goldenFileName : screenshotFileName;
const page = session.browser.getPage(session.id);
return await page.screenshot({ path, ...screenshotOpts });
}

const screenshotFileBuffer = await takeScreenshot();
Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026

Choose a reason for hiding this comment

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

Maybe the TestInfoManager can help and store the error info as well

Copy link
Copy Markdown
Contributor Author

@GZolla GZolla Mar 31, 2026

Choose a reason for hiding this comment

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

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 golden property? Missing golden
  • Is there a byteSize property and pixelDiff is 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.

Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@GZolla GZolla Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@svanherk svanherk Mar 31, 2026

Choose a reason for hiding this comment

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

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.


if (updateGoldens) {
return { pass: true };
}

const rootLength = join(rootDir, PATHS.VDIFF_ROOT).length + 1;

const screenshotFileBuffer = await readFile(screenshotFileName);
const screenshotImage = PNG.sync.read(screenshotFileBuffer);
infoManager.set({
slowDuration: payload.slowDuration,
new: {
height: screenshotImage.height,
path: screenshotFileName.substring(rootLength),
width: screenshotImage.width
}
});

const goldenExists = await checkFileExists(goldenFileName);
if (!goldenExists) {
return { pass: false, message: 'No golden exists. Use the "--golden" CLI flag to re-run and re-generate goldens.' };
}
async function runCompareTest() {
const screenshotImage = PNG.sync.read(screenshotFileBuffer);
infoManager.set({
slowDuration: payload.slowDuration,
new: {
height: screenshotImage.height,
path: screenshotFileName.substring(rootLength),
width: screenshotImage.width
}
});

const goldenFileBuffer = await readFile(goldenFileName);
const goldenImage = PNG.sync.read(goldenFileBuffer);
infoManager.set({
golden: {
height: goldenImage.height,
path: goldenFileName.substring(rootLength),
width: goldenImage.width
const goldenExists = await checkFileExists(goldenFileName);
if (!goldenExists) {
return { pass: false, message: 'No golden exists. Use the "--golden" CLI flag to re-run and re-generate goldens.' };
}
});

if (screenshotImage.width === goldenImage.width && screenshotImage.height === goldenImage.height) {
const goldenSize = (await stat(goldenFileName)).size;
const screenshotSize = (await stat(screenshotFileName)).size;
if (goldenSize === screenshotSize && screenshotFileBuffer.equals(goldenFileBuffer)) {
const success = await tryMoveFile(screenshotFileName, passFileName);
if (!success) return { pass: false, message: 'Problem moving file to "pass" directory.' };
infoManager.set({
new: {
path: passFileName.substring(rootLength)
}
});
return { pass: true };
}
const goldenFileBuffer = await readFile(goldenFileName);
const goldenImage = PNG.sync.read(goldenFileBuffer);
infoManager.set({
golden: {
height: goldenImage.height,
path: goldenFileName.substring(rootLength),
width: goldenImage.width
}
});

if (screenshotImage.width === goldenImage.width && screenshotImage.height === goldenImage.height) {
const goldenSize = (await stat(goldenFileName)).size;
const screenshotSize = (await stat(screenshotFileName)).size;
if (goldenSize === screenshotSize && screenshotFileBuffer.equals(goldenFileBuffer)) {
const success = await tryMoveFile(screenshotFileName, passFileName);
if (!success) return { pass: false, message: 'Problem moving file to "pass" directory.' };
infoManager.set({
new: {
path: passFileName.substring(rootLength)
}
});
return { pass: true };
}

const diff = new PNG({ width: screenshotImage.width, height: screenshotImage.height });
const pixelsDiff = pixelmatch(
screenshotImage.data, goldenImage.data, diff.data, screenshotImage.width, screenshotImage.height, { diffMask: true, threshold: DEFAULT_TOLERANCE }
);

if (pixelsDiff !== 0) {
infoManager.set({
diff: `${screenshotFile.substring(rootLength)}-diff.png`,
pixelsDiff
});
await writeFile(`${screenshotFile}-diff.png`, PNG.sync.write(diff));
return { pass: false, message: `Image does not match golden. ${pixelsDiff} pixels are different.` };
const { diff, pixelsDiff } = getPixelsDiff(screenshotImage, goldenImage);

if (pixelsDiff !== 0) {
infoManager.set({
diff: `${screenshotFile.substring(rootLength)}-diff.png`,
pixelsDiff
});
await writeFile(`${screenshotFile}-diff.png`, PNG.sync.write(diff));
return { pass: false, message: `Image does not match golden. ${pixelsDiff} pixels are different.` };
} else {
infoManager.set({
golden: {
byteSize: goldenSize
},
new: {
byteSize: screenshotSize
},
pixelsDiff
});
return { pass: false, message: 'Image diff is clean but the images do not have the same bytes.' };
}
} else {
infoManager.set({
golden: {
byteSize: goldenSize
},
new: {
byteSize: screenshotSize
},
pixelsDiff
});
return { pass: false, message: 'Image diff is clean but the images do not have the same bytes.' };
return { resizeRequired: true };
}
} else {
return { resizeRequired: true };
}
return await runCompareTest();
} else if (command === 'brightspace-visual-diff-compare-resize') {
const screenshotImage = PNG.sync.read(await readFile(screenshotFileName));
const goldenImage = PNG.sync.read(await readFile(goldenFileName));
Expand All @@ -247,17 +265,14 @@ export function visualDiff({ updateGoldens = false, runSubset = false } = {}) {

let bestIndex = -1;
let bestDiffImage = null;
let pixelsDiff = Number.MAX_SAFE_INTEGER;
let bestPixelsDiff = Number.MAX_SAFE_INTEGER;
for (let i = 0; i < newScreenshots.length; i++) {
const currentDiff = new PNG(newSize);
const currentPixelsDiff = pixelmatch(
newScreenshots[i].png.data, newGoldens[i].png.data, currentDiff.data, currentDiff.width, currentDiff.height, { diffMask: true, threshold: DEFAULT_TOLERANCE }
);
const { diff, pixelsDiff } = getPixelsDiff(newScreenshots[i].png, newGoldens[i].png);

if (currentPixelsDiff < pixelsDiff) {
if (pixelsDiff < bestPixelsDiff) {
bestIndex = i;
bestDiffImage = currentDiff;
pixelsDiff = currentPixelsDiff;
bestDiffImage = diff;
bestPixelsDiff = pixelsDiff;
}
}

Expand All @@ -274,10 +289,10 @@ export function visualDiff({ updateGoldens = false, runSubset = false } = {}) {
new: {
path: `${screenshotFile.substring(rootLength)}-resized-screenshot.png`
},
pixelsDiff
pixelsDiff: bestPixelsDiff
});

return { pass: false, message: `Images are not the same size. When resized, ${pixelsDiff} pixels are different.` };
return { pass: false, message: `Images are not the same size. When resized, ${bestPixelsDiff} pixels are different.` };
}

}
Expand Down