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

US152247 & US152272 - Add pixelmatch and handle images of different sizes #46

Merged
merged 22 commits into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 30 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
"@web/test-runner-commands": "^0.7",
"@web/test-runner-playwright": "^0.10",
"command-line-args": "^5",
"glob": "^10"
"glob": "^10",
"pixelmatch": "^5",
"pngjs": "^7"
}
}
10 changes: 7 additions & 3 deletions src/browser/vdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ mocha.setup({ // eslint-disable-line no-undef
async function ScreenshotAndCompare(opts) {
const name = this.test.fullTitle();
const rect = this.elem.getBoundingClientRect();
const { pass, message } = await executeServerCommand('brightspace-visual-diff', { name, rect, opts });
if (!pass) {
expect.fail(message);
let result = await executeServerCommand('brightspace-visual-diff-compare', { name, rect, opts });
if (result.resizeRequired) {
this.test.timeout(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
this.test.timeout(0);
this.test.timeout(100000);

If we're worried about turning off the timeout, we could also pick a large number. We'll only get to this point after the screenshot is taken, so we don't need to worry about bad tests timing out - those will fail properly. It'd only be if pixelmatch or the file operations never completed that we'd be stuck.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not worried about it -- if we run into these cases we should be able to handle them separately anyway.

result = await executeServerCommand('brightspace-visual-diff-compare-resize', { name });
}
if (!result.pass) {
expect.fail(result.message);
}
}
159 changes: 114 additions & 45 deletions src/server/visual-diff-plugin.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { access, constants, mkdir, readdir, rename, rm, stat } from 'node:fs/promises';
import { access, constants, mkdir, readdir, readFile, rename, rm, writeFile } from 'node:fs/promises';
import { basename, dirname, join } from 'node:path';
import { env } from 'node:process';
import pixelmatch from 'pixelmatch';
import { PNG } from 'pngjs';

const isCI = !!env['CI'];
const DEFAULT_MARGIN = 10;
const DEFAULT_TOLERANCE = 0; // TODO: Support tolerance override?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently allow this to be overridden and a few places have, but I don't think we have a story for it

const PATHS = {
FAIL: 'fail',
GOLDEN: 'golden',
Expand Down Expand Up @@ -56,6 +59,31 @@ async function clearDiffPaths(dir) {
}
}

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
if (original.width === newSize.width && original.height === newSize.height) {
resizedPNGs.push({ png: original, position: `${y.name}-${x.name}` });
Copy link
Contributor Author

@svanherk svanherk Jun 28, 2023

Choose a reason for hiding this comment

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

The position can be adjusted to whatever if we want to use it for the report, I don't actually use it anywhere. I used it for testing, and because it made these nested fors a lot easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the reports are going to need it, since they're going to use the resized images anyway which will always both be the same size.

Copy link
Contributor Author

@svanherk svanherk Jun 29, 2023

Choose a reason for hiding this comment

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

Yeah it was only if we wanted to call it out for extra clarity. I could remove this and switch to just comments for each value, like

	[
		0, // top
		Math.floor((newSize.height - original.height) / 2), // center
		newSize.height - original.height // bottom
	].forEach(y => {
		[
			0, // left
			Math.floor((newSize.width - original.width) / 2), // center
			newSize.width - original.width // right
		].forEach(x => {

?

Copy link
Member

Choose a reason for hiding this comment

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

I guess keep it for now and I'll see if it can be used in a useful way. Your TODO can remind us to remove it later if not!

} 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}` });
}
});
});

return resizedPNGs;
}

async function tryMoveFile(srcFileName, destFileName) {
await mkdir(dirname(destFileName), { recursive: true });
try {
Expand Down Expand Up @@ -101,9 +129,6 @@ export function visualDiff({ updateGoldens = false, runSubset = false } = {}) {
},
async executeCommand({ command, payload, session }) {

if (command !== 'brightspace-visual-diff') {
return;
}
if (session.browser.type !== 'playwright') {
throw new Error('Visual-diff is only supported for browser type Playwright.');
}
Expand All @@ -114,56 +139,100 @@ export function visualDiff({ updateGoldens = false, runSubset = false } = {}) {
const newPath = join(rootDir, PATHS.VDIFF_ROOT, testPath, dir);
const goldenFileName = `${join(newPath, PATHS.GOLDEN, browser, newName)}.png`;
const passFileName = `${join(newPath, PATHS.PASS, browser, newName)}.png`;
const screenshotFileName = `${join(newPath, PATHS.FAIL, browser, newName)}.png`;

if (!isCI) { // CI will be a fresh .vdiff folder each time and only one run
if (session.testRun !== currentRun) {
currentRun = session.testRun;
clearedDirs.clear();
}
const screenshotFile = join(newPath, PATHS.FAIL, browser, newName);
const screenshotFileName = `${screenshotFile}.png`;

if (command === 'brightspace-visual-diff-compare') {
if (!isCI) { // CI will be a fresh .vdiff folder each time and only one run
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about putting this whole cleanup in it's own command to make this cleaner, but then we're running

const browser = session.browser.name.toLowerCase();
const { dir, newName } = extractTestPartsFromName(payload.name);
const testPath = dirname(session.testFile).replace(rootDir, '');
const newPath = join(rootDir, PATHS.VDIFF_ROOT, testPath, dir);
const goldenFileName = `${join(newPath, PATHS.GOLDEN, browser, newName)}.png`;
const passFileName = `${join(newPath, PATHS.PASS, browser, newName)}.png`;
const screenshotFile = join(newPath, PATHS.FAIL, browser, newName);
const screenshotFileName = `${screenshotFile}.png`;
3 times per test. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... was thinking we could also refactor this a bit at this point to be like:

if (command === 'brightspace-visual-diff-compare') {
  doCompare();
} else if (command === 'brightspace-visual-diff-compare-resize') {
  doCompareResize();
}

At the very least that'd reduce a lot of the nesting, and would also let us move things out into separate files if that made sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I started doing that and then there were way too many things I was passing around as parameters without knowing what you'd need and where, so I figured that might be better once the skeleton of the report stuff is in

if (session.testRun !== currentRun) {
currentRun = session.testRun;
clearedDirs.clear();
}

if (runSubset || currentRun > 0) {
if (!clearedDirs.has(newPath)) {
clearedDirs.set(newPath, clearDir(updateGoldens, newPath));
if (runSubset || currentRun > 0) {
if (!clearedDirs.has(newPath)) {
clearedDirs.set(newPath, clearDir(updateGoldens, newPath));
}
await clearedDirs.get(newPath);
}
await clearedDirs.get(newPath);
}
}

const opts = { margin: DEFAULT_MARGIN, ...payload.opts };

const page = session.browser.getPage(session.id);
await page.screenshot({
animations: 'disabled',
clip: {
x: payload.rect.x - opts.margin,
y: payload.rect.y - opts.margin,
width: payload.rect.width + (opts.margin * 2),
height: payload.rect.height + (opts.margin * 2)
},
path: updateGoldens ? goldenFileName : screenshotFileName
});

if (updateGoldens) {
return { pass: true };
}
const opts = { margin: DEFAULT_MARGIN, ...payload.opts };

const page = session.browser.getPage(session.id);
await page.screenshot({
animations: 'disabled',
clip: {
x: payload.rect.x - opts.margin,
y: payload.rect.y - opts.margin,
width: payload.rect.width + (opts.margin * 2),
height: payload.rect.height + (opts.margin * 2)
},
path: updateGoldens ? goldenFileName : screenshotFileName
});

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

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.' };
}
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.' };
}

const screenshotInfo = await stat(screenshotFileName);
const goldenInfo = await stat(goldenFileName);
const screenshotImage = PNG.sync.read(await readFile(screenshotFileName));
const goldenImage = PNG.sync.read(await readFile(goldenFileName));

if (screenshotImage.width === goldenImage.width && screenshotImage.height === goldenImage.height) {
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) {
await writeFile(`${screenshotFile}-diff.png`, PNG.sync.write(diff));
return { pass: false, message: `Image does not match golden. ${pixelsDiff} pixels are different.` };
} else {
const success = await tryMoveFile(screenshotFileName, passFileName);
if (!success) return { pass: false, message: 'Problem moving file to "pass" directory.' };
return { pass: true };
}
} else {
return { resizeRequired: true };
}
} else if (command === 'brightspace-visual-diff-compare-resize') {
const screenshotImage = PNG.sync.read(await readFile(screenshotFileName));
const goldenImage = PNG.sync.read(await readFile(goldenFileName));

const newWidth = Math.max(screenshotImage.width, goldenImage.width);
const newHeight = Math.max(screenshotImage.height, goldenImage.height);
const newSize = { width: newWidth, height: newHeight };

const newScreenshots = await createComparisonPNGs(screenshotImage, newSize);
const newGoldens = await createComparisonPNGs(goldenImage, newSize);

let bestIndex = -1;
let bestDiffImage = null;
let pixelsDiff = 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 }
);

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

// TODO: obviously this isn't how to diff against the golden! Use pixelmatch here.
const same = (screenshotInfo.size === goldenInfo.size);
await writeFile(`${screenshotFile}-resized-screenshot.png`, PNG.sync.write(newScreenshots[bestIndex].png));
await writeFile(`${screenshotFile}-resized-golden.png`, PNG.sync.write(newGoldens[bestIndex].png));
await writeFile(`${screenshotFile}-diff.png`, PNG.sync.write(bestDiffImage));

if (same) {
const success = await tryMoveFile(screenshotFileName, passFileName);
if (!success) return { pass: false, message: 'Problem moving file to pass directory.' };
return { pass: false, message: `Images are not the same size. When resized, ${pixelsDiff} pixels are different.` };
}
return { pass: same, message: 'Does not match golden.' }; // TODO: Add more details once actually diff-ing

}
};
Expand Down
14 changes: 12 additions & 2 deletions test/browser/element.vdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,24 @@ describe('element-different', () => {
elem.style.borderColor = 'black';
elem.text = 'Different Text';
} },
/*{ name: 'smaller', action: elem => {
{ name: 'smaller', action: elem => {
elem.style.width = '200px';
elem.style.height = '50px';
} },
{ name: 'larger', action: elem => {
elem.style.width = '350px';
elem.style.height = '70px';
} }*/
} },
{ name: 'slimer-taller', action: elem => {
elem.style.width = '200px';
elem.style.height = '70px';
elem.style.textAlign = 'end';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding these text-align changes make these last two tests have interesting best positions chosen

} },
{ name: 'wider-shorter', action: elem => {
elem.style.width = '350px';
elem.style.height = '50px';
elem.style.textAlign = 'end';
} }
].forEach(({ name, action }) => {
it(name, async() => {
const elem = await fixture(`<${elementTag} text="Visual Difference"></${elementTag}>`);
Expand Down
1 change: 1 addition & 0 deletions test/browser/nested/element.vdiff.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import '../element.vdiff.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found having this second nested test file was really helpful for all the file manipulation testing, so figured I'd just add it. Could also help with the deleting/creating/rolling up reports testing!

2 changes: 1 addition & 1 deletion test/browser/wtr-vdiff.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { argv } from 'node:process';
import { createConfig } from '../../src/server/wtr-config.js';

const pattern = type => `test/browser/*.${type}.js`;
const pattern = type => `test/browser/**/*.${type}.js`;

function getGoldenFlag() {
return {
Expand Down