feat: add alpha support for PWT snapshot testing #876
Conversation
|
This pull request has been linked to Shortcut Story #17854: Add snapshot support to the CLI. |
clample
force-pushed
the
chrislample/sc-17854
branch
from
5ea330c to
cccd7da
Compare
clample
force-pushed
the
chrislample/sc-17854
branch
from
cccd7da to
7e81874
Compare
clample
force-pushed
the
chrislample/sc-17854
branch
from
7e81874 to
6ee168d
Compare
clample
force-pushed
the
chrislample/sc-17854
branch
from
6ee168d to
7bdc0cf
Compare
| } | ||
| } | ||
|
|
||
| export function detectSnapshots (projectBasePath: string, scriptFilePath: string) { |
There was a problem hiding this comment.
TODO:
Playwright is able to maintain different snapshot files for different platforms (f.ex *-chromium-linux.png for Chromium Linux, which is what we use in our runtimes).
This current implementation will detect all of the snapshots, though, rather than just pushing the Chromium Linux ones.
packages/cli/src/commands/test.ts
Outdated
| if (updateSnapshots) { | ||
| await pullSnapshots(configDirectory, result.assets?.snapshots) | ||
| } |
There was a problem hiding this comment.
Given we will write/update stuff in the local env, maybe we should ask a confirmation from the user at end of the test session
There was a problem hiding this comment.
I'm not so sure. I think that the whole point of --update-snapshots is to pull down the snapshots. I think that the user passing --update-snapshots is already enough of a confirmation to do this. I double checked and PWT also doesn't ask for additional confirmation.
clample
force-pushed
the
chrislample/sc-17854
branch
from
7955a5e to
2f6ce1b
Compare
| @@ -43,6 +43,35 @@ export async function walkDirectory ( | |||
| } | |||
| } | |||
|
|
|||
| export function findFilesRecursively (directory: string, ignoredPaths: Array<string> = []) { | |||
There was a problem hiding this comment.
Don't we have a directory walker somewhere? I think we need a more flexible solution to support multiple scenarios. This is already the case at the runners IMO
There was a problem hiding this comment.
We have walkDirectory here in util.ts. It's unused, though, and calls a callback rather than giving a list of matching files. I'll probably delete it.
findFilesRecursively should be basically the same as the one we have in runners, it's just missing the filePredicate arg. Should I go ahead and proactively add the filePredicate arg? Is there anything else that I should add to this one?
clample
changed the title
This comment has been minimized.
This comment has been minimized.
clample
added
build
|
🎉 Experimental release successfully published on npm |
|
@umutuzgur or @maxigimenez I deleted a leftover from when I was using EventEmitter: 77297fe. Could you please re-approve? |
I hereby confirm that I followed the code guidelines found at engineering guidelines
Affected Components
Notes for the Reviewer
This PR adds initial support for PWT snapshot testing in the CLI .
npx checkly test --update-snapshotsupdates any snapshots using the actual result of this test run.npx checkly deploycan then be used to actually push the snapshots: it updates the snapshots used by the check on Checkly.TODO:
One limitation in the current implementation is that we push the files to storage with every
npx checkly deploy. We could probably improve this by adding some change detection using hashing. Such an implementation would be more complicated, though, so maybe we save it for later.Integration tests are failing since the backend hasn't been deployed with the new schema yet.