-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(ct): replace find-related-test-files
hook with populateDependencies
#31835
chore(ct): replace find-related-test-files
hook with populateDependencies
#31835
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I think we should create a task runner and call |
This comment has been minimized.
This comment has been minimized.
Good call, did that in the new commits. I don't see how |
This comment has been minimized.
This comment has been minimized.
The failing test |
This comment has been minimized.
This comment has been minimized.
@@ -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: [] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Alright, implemented that. What's your take on the questions in #31835 (comment)? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Yeah, these would be separate calls. The idea was that we will use a single extensibility mechanism instead of two. |
I've implemented that, but feel a little unsure about the |
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"2 failed 11 flaky29167 passed, 690 skipped Merge workflow run. |
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 callingsetup
first). Should we also runsetup
? If we do that, does that mean we should also runteardown()
? Should we use a properTaskRunner
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.