-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
tests(): Start moving visual tests to playwright #9481
Conversation
Build Stats
|
#9110 does what you suggest, using a single browser to run all the snapshots. It is very fast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that Polyline code has creeped in from another branch
I would do this differently as explained in the comments
e2e/tests/rendering/index.spec.ts
Outdated
if (testCase.disabled !== 'node') { | ||
await test.step(`node - ${testCase.title}`, async () => { | ||
// we want the browser snapshot of a test to be committed and not the node snapshot | ||
config.config.updateSnapshots = 'none'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong because it will override the -u
flag passed in argv
export async function beforeRenderTest( | ||
cb: ( | ||
canvas: Canvas | ||
) => AsyncReturnValue<{ title: string; boundFunction: () => void }[]>, | ||
options | ||
) { | ||
const el = document.querySelector<HTMLCanvasElement>('#canvas'); | ||
const canvas = new Canvas(el, options); | ||
// cb has to bind the rendering test to the specific canvas and add a clear before the test | ||
const renderingTests = await cb(canvas); | ||
renderingTests.forEach((renderTest) => { | ||
if (renderingTestMap.has(renderTest.title)) { | ||
throw new Error( | ||
`test identifiers must be unique: ${renderTest.title} is already defined` | ||
); | ||
} | ||
renderingTestMap.set(renderTest.title, renderTest.boundFunction); | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I under this.
Do you want to create a new canvas for every test?
Why? What is the gain?
What about simply resizing the canvas before the test and adding a canvas.clear()
after the snapshot call?
Seems more straight forward, simpler and faster and can be exposed on the CanvasUtil.
Motivation
This is an idea / proposal to have something simpler to setup when doing visual tests.
As the old QUNIT-VISUAL we can just prepare a list of functions with some metadata and have them run in sequence.
Those will run opening a single browser window and a single ci action, deprecating the old qunit-visual that runs on 3 node versions and 2 browsers.
The code is a bit messy and is not integrated ( on purpose ) with our generic pattern for single interactive e2e tests.
Eventually also my before function would be moved away from where i added it and just be specific of this kind of test.
I didn't convert more tests because i wanted to talk about it first.
Also is not urgent.