chore(ui): always pass test locations to skip loading unnecessary test files #38235
Conversation
|
The second commit is a breaking change for the VS Code extension, so we can only merge it after the update to the extension was released. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like a problem with windows paths. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }); | ||
| }, [projectFilters, isRunningTest, testModel, testServerConnection, updateSnapshots, singleWorker]); | ||
|
|
||
| const runVisibleTests = React.useCallback(() => runTests('bounce-if-busy', visibleTestIds, new Set(queryParams.args)), [runTests, visibleTestIds]); |
There was a problem hiding this comment.
I am not sure how queryParams.args integrates here. Could you please explain? I'd assume we always use collectTestIds() when running tests, and use queryParams.args when listing to filter things, but I'm not exactly sure.
There was a problem hiding this comment.
i wanted to make it as much of a mechanical refactoring as possible, so I didn't think too closely about the meaning here. We passed queryParams.args into locations before, and we used visibleTestIds here for reasons unbeknownst to me. So I kept it the same.
I refactored it now and was able to remove a bunch of code, so that's great!
Co-authored-by: Dmitry Gozman <[email protected]> Signed-off-by: Simon Knott <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }, [filterText, testModel, statusFilters, projectFilters, runningState, isRunningTest, mergeFiles]); | ||
|
|
||
| const runTests = React.useCallback((mode: 'queue-if-busy' | 'bounce-if-busy', testIds: Set<string>) => { | ||
| const runTests = React.useCallback((mode: 'queue-if-busy' | 'bounce-if-busy', testTree: TestTree, testItems: Iterable<TreeItem>) => { |
There was a problem hiding this comment.
I don't like that we pass TestTree here, especially when it differs from testTree declared on the line 249. Can we pass testIds + locations instead? Perhaps introduce a type for this pair.
| config.cliListOnly = false; | ||
| config.cliPassWithNoTests = true; | ||
| config.cliArgs = params.locations || []; | ||
| config.cliArgs = params.locations; |
There was a problem hiding this comment.
This is not backwards-compatible, so older VSCode extension won't be able to run tests. We should update the extension first.
dgozman
left a comment
dgozman
left a comment
There was a problem hiding this comment.
Changes look good. Can we add tests similar to VSCode ones, where we run tests defined in a helper, outside of testDir, etc?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"1 failed 2431 passed, 116 skipped Merge workflow run. |
Test results for "tests 1"2 failed 4 flaky40291 passed, 787 skipped Merge workflow run. |
|
that actually caught a bug! please take another look. |
2 participants
Mirrors microsoft/playwright-vscode#706 into UI mode.