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

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

Closed
wants to merge 43 commits into from
Closed
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
6769256
Add d2l-test binary
bearfriend Jul 10, 2023
7f73356
Map browser names in getBrowsers
bearfriend Jul 11, 2023
2f59a8a
Cleanup
bearfriend Jul 11, 2023
30d8541
Fix option handling
bearfriend Jul 11, 2023
abefd40
Move WTRConfig class back out of test binary
bearfriend Jul 11, 2023
952140d
Add option aliases
bearfriend Jul 11, 2023
9261e5a
Fix imports
bearfriend Jul 11, 2023
8d3ed90
Delete server/index.js
bearfriend Jul 11, 2023
b1f6faf
Remove HEY!
bearfriend Jul 11, 2023
4640443
Install config-loader
bearfriend Jul 12, 2023
aafbc4e
Remove options that wtr shouldn't use
bearfriend Jul 12, 2023
ebf8ff4
Add --help
bearfriend Jul 12, 2023
923b2f1
Add unit group when used
bearfriend Jul 12, 2023
b7bb29f
Update filter comment
bearfriend Jul 12, 2023
72bd0fa
Add --help details
bearfriend Jul 12, 2023
29dcded
Use proper node imports
bearfriend Jul 12, 2023
b58e57f
Add support for filtering custom group files
bearfriend Jul 12, 2023
8bd61b2
unit -> test
bearfriend Jul 13, 2023
7328376
Remove --manual; Add --open and --slowmo, fixture pausing
bearfriend Jul 13, 2023
7ed8f75
Import pause script; Add start button;
bearfriend Jul 13, 2023
3b910f4
Cleanup
bearfriend Jul 13, 2023
734d226
Cleanup
bearfriend Jul 13, 2023
54b0912
Import pause.js directly
bearfriend Jul 13, 2023
07d2d49
Rewrite
bearfriend Jul 14, 2023
64645e0
Add skip buttons
bearfriend Jul 14, 2023
e84c104
Remember focus element
bearfriend Jul 14, 2023
b068b10
Tweaks
bearfriend Jul 14, 2023
72ee748
Handle RTL
bearfriend Jul 14, 2023
901dc50
Move Skip to right, subtle
bearfriend Jul 14, 2023
2be13d5
Fix chrome headed screenshots
bearfriend Jul 15, 2023
146fabe
Fix firefox headed screenshots
bearfriend Jul 16, 2023
996bc74
Make commands available on window.d2lTest
bearfriend Jul 16, 2023
d5e462b
Add soon chai property
bearfriend Jul 17, 2023
fe9003c
Add retry
bearfriend Jul 17, 2023
9cf177c
Simplify eslint disable rules
bearfriend Jul 17, 2023
b583901
Add root-name + use titlePath
bearfriend Jul 17, 2023
e89bdd7
Cleanup
bearfriend Jul 17, 2023
417a779
Make title grep-friendly when copied
bearfriend Jul 17, 2023
beb4645
Fix safari user-select
bearfriend Jul 17, 2023
05082f6
Remove unnecessary nesting
bearfriend Jul 17, 2023
a493436
Start loading fonts early instead of waiting twice
bearfriend Jul 17, 2023
d0bddca
Remove soon chai property
bearfriend Jul 17, 2023
6fa4680
Remove page load delay
bearfriend Jul 18, 2023
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
154 changes: 154 additions & 0 deletions bin/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
#!/usr/bin/env node
import { ConfigLoaderError, readConfig } from '@web/config-loader';
import commandLineArgs from 'command-line-args';
import commandLineUsage from 'command-line-usage';
import process from 'node:process';
import { startTestRunner } from '@web/test-runner';
import { WTRConfig } from '../src/server/wtr-config.js';

const DISALLOWED_OPTIONS = ['--browsers', '--playwright', '--puppeteer', '--groups', '--manual'];

const optionDefinitions = [
// @web/test-runner options
{
name: 'files',
type: String,
multiple: true,
description: 'Test files to run. Path or glob.\n[Default: ./test/**/*.<group>.js]',
order: 8
},
{
name: 'open',
type: Boolean,
description: 'Open the browser in headed mode'
},
{
name: 'slowmo',
type: Number,
description: 'Slows down test operations by the specified number of milliseconds. Useful so that you can see what is going on.'
},
{
name: 'group',
type: String,
required: true,
defaultOption: true,
description: 'Name of the group to run tests for\n[Default: test]',
order: 1
},
{
name: 'watch',
type: Boolean,
description: 'Reload tests on file changes. Allows debugging in all browsers.',
order: 9
},

// d2l-test options
{
name: 'chrome',
type: Boolean,
description: 'Run tests in Chromium',
order: 2
},
{
name: 'config',
alias: 'c',
type: String,
description: 'Location to read config file from\n[Default: ./d2l-test.config.js]',
order: 9
},
{
name: 'filter',
alias: 'f',
type: String,
multiple: true,
description: 'Filter test files by replacing wildcards with this glob',
order: 6
},
{
name: 'firefox',
type: Boolean,
description: 'Run tests in Firefox',
order: 3
},
{
name: 'golden',
type: Boolean,
description: 'Generate new golden screenshots',
order: 10
},
{
name: 'grep',
alias: 'g',
type: String,
description: 'Only run tests matching this string or regexp',
order: 7
},
{
name: 'help',
type: Boolean,
description: 'Print usage information and exit',
order: 12
},
{
name: 'safari',
type: Boolean,
description: 'Run tests in Webkit',
order: 4
},
{
name: 'timeout',
alias: 't',
type: Number,
description: 'Test timeout threshold in ms\n[Default: 2000]',
order: 5
},
];

const cliArgs = commandLineArgs(optionDefinitions, { partial: true });

if (cliArgs.help) {
const help = commandLineUsage([
{
header: 'D2L Test',
content: 'Test runner for D2L components and applications'
},
{
header: 'Usage',
content: 'd2l-test [options]',
},
{
header: 'Options',
optionList: optionDefinitions
.map(o => (o.description += '\n') && o)
.sort((a, b) => (a.order > b.order ? 1 : -1))
}
]);
process.stdout.write(help);
process.exit();
}

cliArgs._unknown = cliArgs._unknown?.filter(o => !DISALLOWED_OPTIONS.includes(o));

const testConfig = await readConfig('d2l-test.config', cliArgs.config).catch(err => {
if (err instanceof ConfigLoaderError) {
throw new Error(err.message);
} else {
throw err;
}
}) || {};

const wtrConfig = new WTRConfig(cliArgs);
const config = wtrConfig.create(testConfig);

const argv = [
'--group', cliArgs.group,
...(cliArgs._unknown || [])
];
// copy cli-only wtr options back to argv to be processed
cliArgs.watch && argv.push('--watch');

await startTestRunner({
argv,
config,
readFileConfig: false
});
5 changes: 5 additions & 0 deletions package-lock.json

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

10 changes: 7 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@
"test": "npm run lint && npm run test:server && npm run test:browser",
"test:browser": "web-test-runner --files \"./test/browser/**/*.test.js\" --node-resolve --playwright",
"test:server": "mocha ./test/server/**/*.test.js",
"test:vdiff": "web-test-runner --config ./test/browser/wtr-vdiff.config.js --group vdiff",
"test:vdiff": "d2l-test --config ./test/browser/d2l-test.config.js --group vdiff",
"test:vdiff:golden": "npm run test:vdiff -- --golden"
},
"bin": {
"d2l-test": "./bin/test.js"
},
"author": "D2L Corporation",
"license": "Apache-2.0",
"devDependencies": {
Expand All @@ -30,8 +33,7 @@
"sinon": "^15"
},
"exports": {
".": "./src/browser/index.js",
"./wtr-config.js": "./src/server/index.js"
".": "./src/browser/index.js"
},
"files": [
"/src"
Expand All @@ -41,10 +43,12 @@
},
"dependencies": {
"@open-wc/testing": "^3",
"@web/config-loader": "^0.2",
"@web/test-runner": "^0.16",
"@web/test-runner-commands": "^0.7",
"@web/test-runner-playwright": "^0.10",
"command-line-args": "^5",
"command-line-usage": "^7",
"glob": "^10",
"pixelmatch": "^5",
"pngjs": "^7"
Expand Down
1 change: 1 addition & 0 deletions src/browser/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

export async function hoverElem(elem) {
Expand Down
13 changes: 13 additions & 0 deletions src/browser/fixture.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,18 @@ export async function fixture(element, opts = {}) {
await Promise.all([reset(opts), document.fonts.ready]);
const elem = await wcFixture(element);
await waitForElem(elem, opts.awaitLoadingComplete);

await pause();
return elem;
}

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

test.update?.();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test should probably be testControls


if (test.pause) {
await test.pause;
if (test.pause) test.pause = new Promise(r => test.run = r);
}
}
88 changes: 83 additions & 5 deletions src/browser/vdiff.js
Original file line number Diff line number Diff line change
@@ -1,28 +1,106 @@
import { chai, expect } from '@open-wc/testing';
import { chai, expect, nextFrame } from '@open-wc/testing';
import { executeServerCommand } from '@web/test-runner-commands';

let test;
let test, soonPromise;

chai.Assertion.addMethod('golden', function(...args) {
return ScreenshotAndCompare.call({ test, elem: this._obj }, ...args); // eslint-disable-line no-invalid-this
/* eslint-disable no-undef, no-invalid-this */
chai.Assertion.addMethod('golden', async function(...args) {
await soonPromise?.catch(err => expect.fail(err));
return ScreenshotAndCompare.call({ test, elem: this._obj }, ...args);
});
mocha.setup({ // eslint-disable-line no-undef

chai.Assertion.addChainableMethod('soon',
() => { throw new TypeError('"soon" is not a function'); },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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, 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 😆

Copy link
Member

Choose a reason for hiding this comment

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

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.


async function() {
let resolve, reject;
soonPromise = new Promise((res, rej) => {
resolve = res;
reject = rej;
});

try {
expect(this._obj).to.be.instanceof(HTMLElement);
} catch (e) {
reject(e.message);
}

await nextFrame();
await this._obj.updateComplete;
resolve();
}
);

mocha.setup({
rootHooks: {
beforeEach() {
test = this.currentTest;
}
}
});
/* eslint-enable */

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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 it should be.

if (window.d2lTest) {
inlineStyles(this.elem);
document.documentElement.classList.add('screenshot');
}

const name = this.test.fullTitle();
const rect = this.elem.getBoundingClientRect();
let result = await executeServerCommand('brightspace-visual-diff-compare', { name, rect, opts });
if (result.resizeRequired) {
this.test.timeout(0);
result = await executeServerCommand('brightspace-visual-diff-compare-resize', { name });
}

if (window.d2lTest) document.documentElement.classList.remove('screenshot');

if (!result.pass) {
if (window.d2lTest?.pause) {
window.d2lTest.fail();
await window.d2lTest.retryResponse;
}
Comment on lines +42 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

expect.fail(result.message);
}

window.d2lTest?.pass();
}

const disallowedProps = ['width', 'inline-size'];
let count = 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.

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.

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

count += 1;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: invert this and return to reduce nesting


elem.classList.add(`__d2lTestHovering-${count}`);

[...elem.children, ...elem.shadowRoot?.children ?? []].forEach(child => inlineStyles(child));

const computedStyle = getComputedStyle(elem);
[...computedStyle].forEach(prop => {
if (!disallowedProps.includes(prop)) {
elem.style[prop] = computedStyle.getPropertyValue(prop);
}
});

['before', 'after'].forEach(pseudoEl => {
const computedStyle = getComputedStyle(elem, `::${pseudoEl}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (computedStyle.content !== 'none') {
const sheet = new CSSStyleSheet();

const props = [...computedStyle].map(prop => {
const value = computedStyle.getPropertyValue(prop);
return disallowedProps.includes(prop) ? '' : `${prop}: ${value} !important;`;
}).join('');

sheet.insertRule(`.__d2lTestHovering-${count}::${pseudoEl} {${props}}`);
elem.getRootNode().adoptedStyleSheets.push(sheet);
}
});
}
}
13 changes: 9 additions & 4 deletions src/server/headed-mode-plugin.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import { dirname, join } from 'node:path';
import { globSync } from 'glob';

export function headedMode({ manual, watch, pattern }) {
export function headedMode({ open, watch, pattern }) {

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

const delay = ms => ms && new Promise(r => setTimeout(r, ms));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer necessary

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


return {
name: 'brightspace-headed-mode',
async transform(context) {
if ((watch || manual) && files.includes(context.path.slice(1))) {
watch && await new Promise(r => setTimeout(r, 2000));
return `debugger;\n${context.body}`;

if ((watch || open) && files.includes(context.path.slice(1))) {
await delay(0);
const pausePath = join(dirname(import.meta.url), 'pause.js').replace('file:', '');
return `debugger;\nimport '${pausePath}'\n${context.body}`;
}
}
};
Expand Down
1 change: 0 additions & 1 deletion src/server/index.js

This file was deleted.

Loading
Loading