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

svanherk · 2023-06-28T21:29:03Z

Ok, most of the work here was around testing and the resulting code is pretty small. I'll add more comments inline, but at a high-level this PR:

Adds pixelmatch to compare diffs
- If the images are the same size, the diff is created and saved and the test fails
- If the images are different, the screenshot and golden are "grown" accordingly and compared in 9 different positions to find the best diff. This diff is saved, along with the "grown" images for the report and also for local debugging

Other diffing libraries were investigated but in the end pixelmatch was the simplest and gave us what we needed with minimal code.

Co-authored-by: Dave Lockhart <[email protected]>

…spaceUI/testing into US152271_Cleanup_Folders_Option_2

# Conflicts: # src/server/visual-diff-plugin.js # src/server/wtr-config.js

…ncrease_timeout # Conflicts: # src/browser/vdiff.js # src/server/visual-diff-plugin.js # test/browser/element.vdiff.js

svanherk · 2023-06-28T21:32:21Z

src/browser/vdiff.js

-		expect.fail(message);
+	let result = await executeServerCommand('brightspace-visual-diff-compare', { name, rect, opts });
+	if (result.resizeRequired) {
+		this.test.timeout(0);


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.

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

svanherk · 2023-06-28T21:33:05Z

src/server/visual-diff-plugin.js


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


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

svanherk · 2023-06-28T21:34:30Z

src/server/visual-diff-plugin.js

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


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.

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.

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 => {

?

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!

svanherk · 2023-06-28T21:36:05Z

src/server/visual-diff-plugin.js

+			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


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

testing/src/server/visual-diff-plugin.js

Lines 136 to 143 in 9723915

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?

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.

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

svanherk · 2023-06-28T21:39:18Z

test/browser/nested/element.vdiff.js

@@ -0,0 +1 @@
+import '../element.vdiff.js';


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!

svanherk · 2023-06-28T21:39:52Z

test/browser/element.vdiff.js

+		{ name: 'slimer-taller', action: elem => {
+			elem.style.width = '200px';
+			elem.style.height = '70px';
+			elem.style.textAlign = 'end';


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

dlockhart

This looks good! I think we'll want to refactor it at some point, but that can wait.

The tricky part is that we need to add extra data for the report before each of the return statements, which happen all over the place haha.

dlockhart · 2023-06-28T22:39:28Z

src/browser/vdiff.js

-		expect.fail(message);
+	let result = await executeServerCommand('brightspace-visual-diff-compare', { name, rect, opts });
+	if (result.resizeRequired) {
+		this.test.timeout(0);


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

dlockhart · 2023-06-28T22:41:03Z

src/server/visual-diff-plugin.js

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


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.

dlockhart · 2023-06-28T22:43:56Z

src/server/visual-diff-plugin.js

+			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


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.

dlockhart · 2023-06-28T22:46:49Z

src/server/visual-diff-plugin.js

+				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, { threshold: DEFAULT_TOLERANCE }


Can you pass the diffMask option so that they're transparent? I've added it my branch but it keeps getting conflicts/blown away each time I rebase.

ghost · 2023-06-29T19:16:58Z

🎉 This PR is included in version 0.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

svanherk and others added 20 commits June 21, 2023 16:45

Clear appropriate folders during runs

70bc105

Missed cleanup

731aeab

Add comment

11cbfac

Simplify promise code

7a05602

Co-authored-by: Dave Lockhart <[email protected]>

Switch to constants

e3363ad

Merge branch 'US152271_Cleanup_Folders_Option_2' of github.com:Bright…

36ffa98

…spaceUI/testing into US152271_Cleanup_Folders_Option_2

Merge branch 'main' into US152271_Cleanup_Folders_Option_2

baecb3b

Rough idea

56906f4

Merge branch 'main' into US152247_US152272_Add_pixelmatch

3d44569

# Conflicts: # src/server/visual-diff-plugin.js # src/server/wtr-config.js

POC of increasing the timeout if we detect different sizes

9b78b9d

Merge branch 'main' into US152247_US152272_Add_pixelmatch

0cb83f4

Merge branch 'US152247_US152272_Add_pixelmatch' into Add_ability_to_i…

f97053b

…ncrease_timeout # Conflicts: # src/browser/vdiff.js # src/server/visual-diff-plugin.js # test/browser/element.vdiff.js

Renaming and fixes

9224c1a

Mixed test

0f05804

Fixes

1cbd9d0

Merge branch 'main' into US152247_US152272_Add_pixelmatch

5c6bb05

Cleanup

a9e75fc

Add nested file for testing

5a2201a

More tests

828a341

Turn off timeouts

9723915

svanherk requested a review from a team as a code owner June 28, 2023 21:29

svanherk commented Jun 28, 2023

View reviewed changes

Adjust variable name

4328f4a

dlockhart approved these changes Jun 28, 2023

View reviewed changes

Add diffmask

77435e4

dlockhart approved these changes Jun 29, 2023

View reviewed changes

svanherk merged commit 7af4701 into main Jun 29, 2023
1 check passed

svanherk deleted the US152247_US152272_Add_pixelmatch branch June 29, 2023 19:15

ghost added the released label Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

svanherk commented Jun 28, 2023 •

edited

Loading

svanherk Jun 28, 2023

dlockhart Jun 28, 2023

svanherk Jun 28, 2023

svanherk Jun 28, 2023 •

edited

Loading

dlockhart Jun 28, 2023

svanherk Jun 29, 2023 •

edited

Loading

dlockhart Jun 29, 2023

svanherk Jun 28, 2023

dlockhart Jun 28, 2023

svanherk Jun 29, 2023

svanherk Jun 28, 2023

svanherk Jun 28, 2023

dlockhart left a comment

dlockhart Jun 28, 2023

dlockhart Jun 28, 2023

dlockhart Jun 28, 2023

dlockhart Jun 28, 2023

ghost commented Jun 29, 2023

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

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

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

Conversation

svanherk commented Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlockhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 29, 2023

svanherk commented Jun 28, 2023 •

edited

Loading

svanherk Jun 28, 2023 •

edited

Loading

svanherk Jun 29, 2023 •

edited

Loading