chore(ct): replace `find-related-test-files` hook with `populateDependencies` #31835

Skn0tt · 2024-07-24T06:33:33Z

As discussed in the team sync, now that we have the populateDependencies plugin step, we don't need this CLI hook anymore.

I'm not sure if this refactor is worth it though, because it means we'll start calling the Plugin steps out of order (we call populateDependencies without calling setup first). Should we also run setup? If we do that, does that mean we should also run teardown()? Should we use a proper TaskRunner instance for managing that? Or should we just stick with the hook we have for simplicity over architectural conformance?

Lots of questions that i'm sure @dgozman has great answers to :D

This is a follow-up to #31727.

…cies

dgozman · 2024-07-24T09:14:54Z

I think we should create a task runner and call plugin.setup/teardown as usual. And we can also migrate clear-cache and dev-server to do the same.

Skn0tt · 2024-07-24T11:49:46Z

Good call, did that in the new commits.

I don't see how clear-cache and dev-server are related to populateDependencies. Are you saying we should add new Plugin steps for that, just like populateDependencies?

Skn0tt · 2024-07-24T11:55:26Z

The failing test run-tests.spec.ts:1153:5 › should produce output twice also fails on main and I don't see how it'd be related to this change, so I'm ignoring it.

dgozman · 2024-07-24T12:30:54Z

packages/playwright/src/runner/runner.ts

@@ -138,14 138,10 @@ export class Runner {
 }

 async findRelatedTestFiles(mode: 'in-process' | 'out-of-process', files: string[]): Promise<FindRelatedTestFilesReport> {
- const result = await this.loadAllTests(mode);
- if (result.status !== 'passed' || !result.suite)
- return { errors: result.errors, testFiles: [] };


I think we should keep reporting the errors.

Note that you can modify loadAllTests() to make it reusable for this case. I also noticed that we don't need lines 124-130 (old code) at all, so it can be simpler as well.

Skn0tt · 2024-07-24T13:04:41Z

Alright, implemented that. What's your take on the questions in #31835 (comment)?

dgozman · 2024-07-25T13:44:33Z

I don't see how clear-cache and dev-server are related to populateDependencies. Are you saying we should add new Plugin steps for that, just like populateDependencies?

Yeah, these would be separate calls. The idea was that we will use a single extensibility mechanism instead of two.

Skn0tt · 2024-07-25T15:13:48Z

I've implemented that, but feel a little unsure about the runDevServer hook. In order for it to work, we need to pass over FullConfigInternal. Is Plugin part of our public API? If it is, giving it an internal config sounds like a no-go.

github-actions · 2024-07-25T16:28:10Z

Test results for "tests 1"

2 failed
❌ [playwright-test] › runner.spec.ts:118:5 › should ignore subprocess creation error because of SIGINT
❌ [playwright-test] › web-server.spec.ts:153:5 › should resolve cwd wrt config directory

11 flaky

⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [chromium-library] › library/trace-viewer.spec.ts:248:1 › should have network requests
⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [firefox-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:42:3 › get should work
⚠️ [firefox-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [playwright-test] › ui-mode-test-setup.spec.ts:87:5 › should show errors in config
⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots
⚠️ [webkit-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all

29167 passed, 690 skipped
✔️✔️✔️

Merge workflow run.

chore(ct): replace find-related-test-files hook with populateDependen…

2d8c3d6

…cies

Skn0tt self-assigned this Jul 24, 2024