test(client-snapshots): restore update scripts #25756
Conversation
- rollback common ignore to allow per command line specification - update test output to show incremental progress for long running tests
github-actions
bot
added
base: main
There was a problem hiding this comment.
Pull Request Overview
This PR restores the ability to run snapshot generation and update scripts by reverting common ignore patterns and allowing per-command-line specification. It also adds progress logging to provide incremental feedback during long-running tests.
Key changes:
- Removed the common ignore pattern from
.mocharc.cjsto enable flexible test file filtering per script - Added timing and progress logging to
processOneNodeto show test execution duration - Enhanced data immutability by using
Readonlytype andas const satisfiespattern
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/test/snapshots/src/replayMultipleFiles.ts | Added name field to IWorkerArgs, implemented progress logging with timing, and improved immutability patterns |
| packages/test/snapshots/package.json | Updated test scripts to explicitly specify file patterns and removed --experimental-worker flag |
| packages/test/snapshots/.mocharc.cjs | Removed default spec pattern to allow individual scripts to specify their own test file filters |
| snapFreq, | ||
| testSummaries, | ||
| }; | ||
| } as const satisfies IWorkerArgs; |
There was a problem hiding this comment.
Using as const satisfies IWorkerArgs creates an immutable object that cannot be spread or modified later. This conflicts with the code at lines 245-249 where data is spread using ...data. Consider using satisfies IWorkerArgs without as const to maintain type safety while allowing the object to be spread.
| } as const satisfies IWorkerArgs; | |
| } satisfies IWorkerArgs; |
There was a problem hiding this comment.
Huh, an immutable object can't be spread? Sure seems like it can.
instead of timing out
| } | ||
|
|
||
| const p = worker().then(() => { | ||
| const p = worker().finally(() => { |
There was a problem hiding this comment.
this seems like the kind of change that should have a unit test, since if it breaks, it could do so in a way that silently hides errors.
As I see no changes to the tests in this package, I assume this change does not have unit test coverage. Such tests could also help document the intended behavior and usage.
There was a problem hiding this comment.
This is not a normal package. There are no exports and no scripts. All of the "src" code is test implementation.
I considered moving it all under src/test but the recent change to execute all files under src/test except those with .tool in the name discouraged that.
2 participants
Uh oh!
There was an error while loading. Please reload this page.