feat(test-runner-visual-regression): add testName to GetNameArgs by WickyNilliams · Pull Request #1657 · modernweb-dev/web

WickyNilliams · 2021-09-01T12:25:13Z

by supplying test name to each of the extensibility points, it becomes easier to store images relative to the test itself.

For example, this will store snapshot images in a directory adjacent to the test file:

visualRegressionPlugin({
  getBaselineName: ({ name, browser, testFile }) =>
    path.join(testFile, "..", "screenshots", browser, "baseline", name),

  getDiffName: ({ browser, name, testFile }) =>
    path.join(testFile, "..", "screenshots", browser, "failed", `${name}-diff`),
    
  getFailedName: ({ browser, name, testFile }) =>
    path.join(testFile, "..", "screenshots", browser, "failed", name),
});

What I did

Added testFile property to GetNameArgs
Passed session.testFile from plugin through to extensibility points like getBaselineName etc
Updated README

changeset-bot · 2021-09-01T12:25:16Z

🦋 Changeset detected

Latest commit: fa6f894

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@web/test-runner-visual-regression	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

web-padawan

In general the approach looks good to me 👍 By the way, I was previously asking for this in #1040

Please consider implementing my suggestion about changing visualDiff command arguments.
And also please add a changeset for this PR using npx changeset to trigger a version bump.

If you think that my suggestion does make sense, I think the bump cab be minor.
But technically it seems to be a non-breaking change so it can be a patch during 0.x

web-padawan

Please rebase as there are merge conflicts now, also please check my comment.

WickyNilliams · 2021-09-02T09:34:20Z

@web-padawan yeah i'll rebase now and make the changes you suggested

By supplying test name to each of the extensibility points, it becomes easier to store images relative to the test itself. Clean up param list for visual diff command, add new VisualDiffCommandContext type

WickyNilliams · 2021-09-02T09:57:53Z

Added changeset, made changes you suggested, rebased, squashed. I went with exactly the changes you suggested instead of further tweaking things. Let me know if it's all good

web-padawan

I have updated the changeset to use patch as this does not affect public API of a plugin.

web-padawan

LGTM 💯

Waiting for CI to pass and then I will merge and release a new version.
Thank you for your valuable contributions to this project, I really appreciate it.

web-padawan · 2021-09-02T11:14:50Z

CI failure is unrelated, see #1577. Restarted, hopefully this time it passes 🤞

web-padawan · 2021-09-02T11:24:20Z

Another unrelated failure, sigh. This also happens from time to time:

Failed to execute command test:ci for 3 packages. But we don't know which ones, because concurrently doesn't say.

[dev-server-import-maps] Error: listen EADDRINUSE: address already in use :::8000
[dev-server-import-maps]     at Server.setupListenHandle [as _listen2] (net.js:1316:16)
[dev-server-import-maps]     at listenInCluster (net.js:1364:12)
[dev-server-import-maps]     at Server.listen (net.js:1450:7)
[dev-server-import-maps]     at internal/util.js:297:30
[dev-server-import-maps]     at new Promise (<anonymous>)
[dev-server-import-maps]     at Server.<anonymous> (internal/util.js:296:12)
[dev-server-import-maps]     at DevServer.start (/home/runner/work/web/web/packages/dev-server-core/dist/server/DevServer.js:36:69)
[dev-server-import-maps]     at TestRunnerServer.start (/home/runner/work/web/web/packages/test-runner-core/dist/server/TestRunnerServer.js:49:30)
[dev-server-import-maps]     at TestRunner.start (/home/runner/work/web/web/packages/test-runner-core/dist/runner/TestRunner.js:56:31)
[dev-server-import-maps]     at Object.startTestRunner (/home/runner/work/web/web/packages/test-runner/dist/startTestRunner.js:49:22) {
[dev-server-import-maps]   code: 'EADDRINUSE',
[dev-server-import-maps]   errno: 'EADDRINUSE',
[dev-server-import-maps]   syscall: 'listen',
[dev-server-import-maps]   address: '::',
[dev-server-import-maps]   port: 8000
[dev-server-import-maps] }

Restarted again 🙏

WickyNilliams · 2021-09-02T12:46:52Z

I have updated the changeset to use patch as this does not affect public API of a plugin.

It felt like a new feature (of sorts) to me, that's why i went for a minor version. But that's your call

Thank you for your valuable contributions to this project, I really appreciate it.

No worries. Thanks for reviewing and merging so quickly! I will probably continue to contribute fixes and improvements as needs arise :) Thanks again

web-padawan · 2021-09-02T13:22:49Z

It felt like a new feature (of sorts) to me, that's why i went for a minor version. But that's your call

According to semver, during 0.x we use minor versions to indicate breaking changes so I decided to change it.

Sure, keep these fixes coming if you find something that needs improvement 🙇‍♂️

WickyNilliams · 2021-09-02T14:54:57Z

Didn't realise that subtlety of v0.x! Good to know.

WickyNilliams commented Sep 1, 2021

View reviewed changes

Comment thread packages/test-runner-visual-regression/src/visualRegressionPlugin.ts Outdated

web-padawan reviewed Sep 2, 2021

View reviewed changes

Comment thread packages/test-runner-visual-regression/src/visualRegressionPlugin.ts Outdated

web-padawan requested changes Sep 2, 2021

View reviewed changes

web-padawan mentioned this pull request Sep 2, 2021

Version Packages #1658

Merged

feat(test-runner-visual-regression): add testName to GetNameArgs

80a3f36

By supplying test name to each of the extensibility points, it becomes easier to store images relative to the test itself. Clean up param list for visual diff command, add new VisualDiffCommandContext type

WickyNilliams force-pushed the feat/visual-diff-supply-test-name branch from 81a23dd to 80a3f36 Compare September 2, 2021 09:56

chore: update changeset to use patch

fa6f894

web-padawan reviewed Sep 2, 2021

View reviewed changes

web-padawan approved these changes Sep 2, 2021

View reviewed changes

web-padawan merged commit 395c605 into modernweb-dev:master Sep 2, 2021

WickyNilliams deleted the feat/visual-diff-supply-test-name branch September 2, 2021 12:46

web-padawan mentioned this pull request Sep 10, 2021

test: simplify visual tests to not use import.meta vaadin/web-components#2506

Merged

Uh oh!

Conversation

WickyNilliams commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

Uh oh!

changeset-bot bot commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

web-padawan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

web-padawan left a comment

Choose a reason for hiding this comment

Uh oh!

WickyNilliams commented Sep 2, 2021

Uh oh!

WickyNilliams commented Sep 2, 2021

Uh oh!

web-padawan left a comment

Choose a reason for hiding this comment

Uh oh!

web-padawan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

web-padawan commented Sep 2, 2021

Uh oh!

web-padawan commented Sep 2, 2021

Uh oh!

WickyNilliams commented Sep 2, 2021

Uh oh!

web-padawan commented Sep 2, 2021

Uh oh!

WickyNilliams commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WickyNilliams commented Sep 1, 2021 •

edited

Loading

changeset-bot bot commented Sep 1, 2021 •

edited

Loading

web-padawan left a comment •

edited

Loading

web-padawan left a comment •

edited

Loading