Feat watch mode - #804
Conversation
commit: |
✅ Component tests succeed
|
✅ E2E tests succeed
|
62eb636 to
54369aa
Compare
KuznetsovRoman
left a comment
There was a problem hiding this comment.
I would appreciate more modular structure, where tests watcher would be pulled to separate module with short and concise module calls instead of being splattered all over the code base.
|
|
||
| await app.initialize(); | ||
|
|
||
| if (args.cli.options.watch && toolAdapter.toolName === ToolName.Testplane) { |
There was a problem hiding this comment.
Its not the clean architecture i would like to see in html-reporter.
We have a lot of layers in html-reporter (server launches app, which manages tool-runner) to distribute responsibility between the layers, and you just pasted entire watcher in "gui/server.ts".
Now "server.ts" handles chokidar, calculates scope, manages debounce, drain the que, manages this "relatively low" level error handling...
Its clearly should be another module, and not "server.ts" responsibility.
There was a problem hiding this comment.
Fixed. Moved watcher logic to separate TestsWatcher module. Now server.ts only creates and starts it.
| const watchPaths = [...new Set([...config.getTestFilePatterns(), ...args.paths])]; | ||
| const watchRoots = [...new Set(watchPaths.map(getWatchRoot).filter(Boolean))]; |
There was a problem hiding this comment.
You are building watch roots, that may differ from real testplane scope:
- If we have empty default set with
files: [], your code gets empty array, while testplane reads default test directories - When running
npx testplane gui tests/a.ts --watch, watches subscribes on all of the config globs, which is not what should be done here. - Selecting set through CLI call also is not considered
- If path is set like "tests" without ending "/" or asterisks on the end, your "path.dirname('tests')" returns ".", so the watcher looks at all project files, including node_modules
Examples:
• getWatchRoot('tests') -> .
• getWatchRoot('tests/') -> tests
• getWatchRoot('tests/**/*.ts') -> tests
As a solution i see "creating watch plan in testplane adapter the same way test set is built for initial read". So it would be single source of truth, and not "couple of testfile finding mechanisms, which can work differently"
There was a problem hiding this comment.
Fixed. Watch plan is now created in Testplane adapter. It handles CLI paths, selected sets, environment sets and default folders. Added tests for this cases.
| let refreshSequence = 0; | ||
| let firstDebouncedEventAt: number | undefined; | ||
|
|
||
| const refresh = async (changedFiles: string[], removedDirectories: string[]): Promise<void> => { |
There was a problem hiding this comment.
There is a "catch" (thats a wordplay) in this function:
If some step, like, retrieving sqlite from history, fails, browser would only get "TESTS_REFRESH_FAILED" without actual tree patch. So if there was test A, which was renamed to test B, after the error browser would see test A, not test B, despite server collection would have the opposite: test B, not test A.
Trying to run test A would likely result in silent "nothing is really launched"
There was a problem hiding this comment.
Fixed. Collection is updated only after successful tree update. If update fails, previous tree, test adapters and attempts are restored. Added test for this case.
| } | ||
| } | ||
| const isChangedFile = (test: TestAdapter): boolean => affectedFiles.has(path.resolve(test.file)); | ||
| const signature = (test: TestAdapter): string => JSON.stringify([test.browserId, path.resolve(test.file), test.titlePath]); |
There was a problem hiding this comment.
Your signature only includes "browserId", "resolved file" and "titlePath"
But tree building also consider "disabled", "silentlySkipped" and "pending"
For example, changing "it" to "it.skip" would have the exact signature and therefore would not refresh.
| if (filesToRead.length) { | ||
| try { | ||
| stageStartedAt = performance.now(); | ||
| changedCollection = await this._toolAdapter.readTests(filesToRead, this._globalOpts); |
There was a problem hiding this comment.
Shouldn't you separately check for "unique full name" here?
"readTests" under the hood checks for unique full names only for tests it currently read.
But you read only subset of tests and then combine it with previous tree without checking for fullName duplicates. It could potentially lead into some hard-to-debug problem.
There was a problem hiding this comment.
Fixed. Partial update now checks duplicate test names in changed and unchanged files. Added test for this case.
| items: Object.keys(event.data.data).length | ||
| }); | ||
| } | ||
| self.postMessage(true); |
There was a problem hiding this comment.
So now worker sends "true" on both "init" and "patch"
Existing "search" function in this file expects array of strings
If we get "true" from worker after search was initiated, "search" could confuse its response as "response to search" and fail due to "TypeError: array expected" or something like that.
I think we should move to typed messages: dispatching not just single "true", but whole object with "type" field in it in order to avoid confusion.
| } | ||
| }); | ||
|
|
||
| worker.postMessage({ |
There was a problem hiding this comment.
Have you checked that "Test DOES appear when there was a filter, preventing from showing it, because its old name did not suite the filter, but the new one - does"?
There was a problem hiding this comment.
Fixed. Worker messages now have type and requestId, so different responses are not mixed. Added test for this case.
| } | ||
| }; | ||
|
|
||
| testsWatcher = chokidar.watch(watchPaths, { |
There was a problem hiding this comment.
So, "chokidar.watch" errors crash the whole gui process. Is it expected? Is it like super stable so we wont expect it ever happening?
I would add some kind of "catch", at least for easier debugging (when somebody would see scary error message from chokidar.watch exception, he should at least understand that the error was because of some king of error in tree watcher)
There was a problem hiding this comment.
Fixed. Current search filter runs again after search index update. Added test for renamed test case.
| for (const {suitePath, browserId} of uniqueTests) { | ||
| const statement = this._db.prepare( | ||
| `SELECT * FROM ${DB_SUITES_TABLE_NAME} WHERE suitePath = ? AND name = ?` | ||
| ); |
There was a problem hiding this comment.
Why are you doing N full scans? Its not clickhouse with its magic data skipping indexes. Its better to iterate through every "suite" once and filter everything you need by yourself, instead of torturing SQLite like that.
54369aa to
4251ed5
Compare
1926485 to
dff7146
Compare
| cwd: process.cwd(), | ||
| ignoreInitial: true, | ||
| ignored: [ | ||
| /(^|[/\\])\../, |
There was a problem hiding this comment.
Looks like it would ignore .tests/case.ts
We actually have some concrete examples: https://nda.ya.ru/t/VVkNi_VO7qaK7o
| const getTestStructureSignature = (test: TestAdapter): string => JSON.stringify([ | ||
| test.browserId, | ||
| path.resolve(test.file), | ||
| test.titlePath, | ||
| test.disabled, | ||
| test.silentlySkipped, | ||
| test.pending | ||
| ]); |
There was a problem hiding this comment.
Maybe we should also add skip reason here?
Now skip reason update would not update real test in the tree because its not a part of the key
| logger.log(`[watch-perf][server][#${refreshId}] serialize/write SSE: ${(performance.now() - sendStartedAt).toFixed(1)}ms`); | ||
| } | ||
| logger.log(`[watch-perf][server][#${refreshId}] refresh loop total: ${(performance.now() - refreshStartedAt).toFixed(1)}ms ${JSON.stringify({changed})}`); |
There was a problem hiding this comment.
Those type of logs would be removed before merging, right?
If you want to leave them in the code, you should at least hide them in "debug"
There was a problem hiding this comment.
Yea, I will remove it, it is just for review and tests
Watch mode for gui mode.
Usage:
npx testplane gui --watchScreen.Recording.2026-09-03.at.01.35.28.mov