Add fixture pausing and --open option to replace --manual #71

bearfriend · 2023-07-13T12:43:44Z

US152298: Visual-diff: ability to debug in the browser

Removes --manual option (still accessible via --watch)
Adds --open option to run tests in headed playwright browsers without --watch
Fixes various issues with taking screenshots in headed Chrome and both headed and headless Firefox
Adds fixture pausing in headed browser modes with UI controls to
- start or skip test files
- run or skip individual tests
- retry failed tests
- run all remaining tests

package.json

src/server/pause.js

src/server/wtr-config.js

src/server/headed-mode-plugin.js

bearfriend · 2023-07-17T13:56:19Z

src/browser/commands.js

@@ -30,6 +30,7 @@ export async function focusElem(elem) {

 export async function hoverAt(x, y) {
 	await sendMouse({ type: 'move', position: [x, y] });
+	if (window.d2lTest) window.d2lTest.hovering = true;


Indicates to screenshotAndCompare that the current test performed a hover. For Chrome, this means we need to move all styles inline for the screenshot.

bearfriend · 2023-07-17T13:58:03Z

src/browser/fixture.js

+async function pause() {
+	const test = window.d2lTest || {};
+
+	test.update?.();


test should probably be testControls

bearfriend · 2023-07-17T14:00:41Z

src/browser/vdiff.js

 });
-mocha.setup({ // eslint-disable-line no-undef
+
+chai.Assertion.addChainableMethod('soon',


This lets you do expect(elem).to.be.soon.golden() to delay comparing until the next frame + an updateComplete. I found this was necessary in Firefox for some of the vdiff tests in core.

Interesting. Did they share anything in common with each other? My worry with adding this would be that it'd become a "silver bullet" for flaky tests, when in reality the problem was more related to the consumer not properly awaiting updateComplete after making an update or not awaiting an event after triggering a change.

Yeah, that's totally fair. The (mostly) common thread was changing things outside the component itself that would cause it to update (like scrolling or inserting HTML into the fixture's light DOM), but not until the next frame, so updateComplete isn't necessarily doing anything helpful here, but seemed good to include, except when you consider the potential for misuse.

Anyway, I think we can get rid of it and change these floating-buttons test like this:

// fixed with oneEvent() it('does not float at bottom of container', async() => { const elem = await fixture(floatingButtonsFixture); window.scrollTo(0, document.body.scrollHeight); await oneEvent(window, 'scroll'); await expect(elem).to.be.golden(); }); // doesn't make sense to me that this one would need a nextFrame(), but it does fix it it('does not float when small amount of content', async() => { const elem = await fixture(floatingButtonsShortFixture); await nextFrame(); await expect(elem).to.be.golden(); }); // fixed with nextFrame() it('floats when content added to dom', async() => { const elem = await fixture(floatingButtonsShortFixture); const contentElem = document.querySelector('#floating-buttons-short-content').querySelector('p'); contentElem.innerHTML += '<br><br>I love Coffee...'; await nextFrame(); await expect(elem).to.be.golden(); });

To be honest I kind of just wanted to get a custom chai property to work 😆

Haha cool. Yeah I'm totally fine with tests needing to await nextFrame() themselves if they do things that would require waiting for a repaint. At least that's logical.

bearfriend · 2023-07-17T14:08:58Z

src/browser/vdiff.js

+			window.d2lTest.fail();
+			await window.d2lTest.retryResponse;


Rejects the result promise being await-ed to show the Retry button.

bearfriend · 2023-07-17T14:13:17Z

src/browser/vdiff.js

+	window.d2lTest?.pass();
+}
+
+const disallowedProps = ['width', 'inline-size'];


I don't love this, since width in particular could be a hover style, but setting the computed width causes issues in grid layouts like lists. We could get fancier down the line as-needed.

Also, I think the headed tests passing is ok as "best effort". It's really more to step through and debug anyway.

bearfriend · 2023-07-17T14:16:07Z

src/browser/vdiff.js

+			}
+		});
+
+		['before', 'after'].forEach(pseudoEl => {


Pesudo elements are... pseudo, so they aren't children and can't have inline styles.

bearfriend · 2023-07-17T14:17:10Z

src/server/headed-mode-plugin.js


 	const files = globSync(pattern, { ignore: 'node_modules/**', posix: true });

+	const delay = ms => ms && new Promise(r => setTimeout(r, ms));


No longer necessary

Suggested change

const delay = ms => ms && new Promise(r => setTimeout(r, ms));

bearfriend · 2023-07-17T14:19:45Z

src/server/pause.js

I think this can mostly be turned into a web component to make it less gross (scoped styles, less window, oop etc.), but it would require a fair amount of rework that we can do later.

bearfriend · 2023-07-17T14:24:44Z

src/browser/vdiff.js


 async function ScreenshotAndCompare(opts) {
+	await document.fonts.ready; // firefox fonts


For some reason Firefox fonts go back into loading, so we need to double check they are done before the screenshot.

Weird! Hopefully this is a no-op in non-Firefox anyway.

Yeah it should be.

dlockhart · 2023-07-17T14:35:35Z

src/browser/vdiff.js

 });
-mocha.setup({ // eslint-disable-line no-undef
+
+chai.Assertion.addChainableMethod('soon',


Interesting. Did they share anything in common with each other? My worry with adding this would be that it'd become a "silver bullet" for flaky tests, when in reality the problem was more related to the consumer not properly awaiting updateComplete after making an update or not awaiting an event after triggering a change.

dlockhart · 2023-07-17T14:36:04Z

src/browser/vdiff.js


 async function ScreenshotAndCompare(opts) {
+	await document.fonts.ready; // firefox fonts


Weird! Hopefully this is a no-op in non-Firefox anyway.

dlockhart · 2023-07-17T14:38:27Z

src/browser/vdiff.js

+function inlineStyles(elem) {
+	// headed chrome takes screenshots by first moving the element, which
+	// breaks the hover state. So, copy current styles inline before screenshot
+	if (window.d2lTest.hovering && window.chrome) {


Nit: invert this and return to reduce nesting

dlockhart · 2023-07-17T14:40:11Z

src/browser/vdiff.js

+let count = 0;
+function inlineStyles(elem) {
+	// headed chrome takes screenshots by first moving the element, which
+	// breaks the hover state. So, copy current styles inline before screenshot


Whoa, crazy that we'd need to do this. I'm sure you tried this, but instead of making all the styles inline, could we just try to re-apply the hover after Chrome moves it?

Yeah, it's not just that it moves. It seems to be locking it down in some way, which makes sense, but it just loses the hover.

bearfriend · 2023-07-19T13:15:57Z

Since the base (#65) got reworked the conflicts are a bit much, and this is a doing lots of smaller things so I'll just close this and re-open the individual bits as their own PRs.

bearfriend added 20 commits July 11, 2023 13:13

Add d2l-test binary

6769256

Map browser names in getBrowsers

7f73356

Cleanup

2f59a8a

Fix option handling

30d8541

Move WTRConfig class back out of test binary

abefd40

Add option aliases

952140d

Fix imports

9261e5a

Delete server/index.js

8d3ed90

Remove HEY!

b1f6faf

Install config-loader

4640443

Remove options that wtr shouldn't use

aafbc4e

Add --help

ebf8ff4

Add unit group when used

923b2f1

Update filter comment

b7bb29f

Add --help details

72bd0fa

Use proper node imports

29dcded

Add support for filtering custom group files

b58e57f

unit -> test

8bd61b2

Remove --manual; Add --open and --slowmo, fixture pausing

7328376

Import pause script; Add start button;

7ed8f75

bearfriend marked this pull request as ready for review July 13, 2023 12:43

bearfriend requested a review from a team as a code owner July 13, 2023 12:43

Cleanup

3b910f4

dlockhart reviewed Jul 13, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/server/pause.js Outdated Show resolved Hide resolved

src/server/pause.js Outdated Show resolved Hide resolved

src/server/wtr-config.js Show resolved Hide resolved

dlockhart reviewed Jul 13, 2023

View reviewed changes

src/server/headed-mode-plugin.js Outdated Show resolved Hide resolved

bearfriend added 2 commits July 13, 2023 12:05

Cleanup

734d226

Import pause.js directly

54b0912

bearfriend force-pushed the dgleckler/fixture-pause branch from 60d929d to 54b0912 Compare July 13, 2023 17:33

bearfriend added 2 commits July 13, 2023 21:18

Rewrite

07d2d49

Add skip buttons

64645e0

bearfriend added 2 commits July 14, 2023 00:46

Remember focus element

e84c104

Tweaks

b068b10

bearfriend force-pushed the dgleckler/fixture-pause branch from 5d0ecb3 to b068b10 Compare July 14, 2023 13:12

bearfriend added 11 commits July 14, 2023 09:46

Handle RTL

72ee748

Move Skip to right, subtle

901dc50

Fix chrome headed screenshots

2be13d5

Fix firefox headed screenshots

146fabe

Make commands available on window.d2lTest

996bc74

Add soon chai property

d5e462b

Add retry

fe9003c

Simplify eslint disable rules

9cf177c

Add root-name + use titlePath

b583901

Cleanup

e89bdd7

Make title grep-friendly when copied

417a779

bearfriend commented Jul 17, 2023

View reviewed changes

Fix safari user-select

beb4645

dlockhart reviewed Jul 17, 2023

View reviewed changes

bearfriend added 4 commits July 17, 2023 19:49

Remove unnecessary nesting

05082f6

Start loading fonts early instead of waiting twice

a493436

Remove soon chai property

d0bddca

Remove page load delay

6fa4680

bearfriend mentioned this pull request Jul 19, 2023

Wrap @web/test-runner binary #65

Merged

bearfriend force-pushed the dgleckler/bin-wrap branch from 0829a96 to b2d4060 Compare July 19, 2023 12:22

bearfriend closed this Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fixture pausing and --open option to replace --manual #71

Add fixture pausing and --open option to replace --manual #71

bearfriend commented Jul 13, 2023 •

edited

Loading

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

dlockhart Jul 17, 2023

bearfriend Jul 17, 2023

dlockhart Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend Jul 17, 2023

dlockhart Jul 17, 2023

bearfriend Jul 17, 2023

dlockhart Jul 17, 2023

dlockhart Jul 17, 2023

dlockhart Jul 17, 2023

dlockhart Jul 17, 2023

bearfriend Jul 17, 2023

bearfriend commented Jul 19, 2023 •

edited

Loading


		const files = globSync(pattern, { ignore: 'node_modules/**', posix: true });

		const delay = ms => ms && new Promise(r => setTimeout(r, ms));


		async function ScreenshotAndCompare(opts) {
		await document.fonts.ready; // firefox fonts

Add fixture pausing and --open option to replace --manual #71

Add fixture pausing and --open option to replace --manual #71

Conversation

bearfriend commented Jul 13, 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

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

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

bearfriend commented Jul 19, 2023 • edited Loading

bearfriend commented Jul 13, 2023 •

edited

Loading

bearfriend commented Jul 19, 2023 •

edited

Loading