-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Add -failfast flag when the mode is fail_fast #15790
Conversation
6dbdd8a
to
057a0e2
Compare
Signed-off-by: Rajalakshmi Girish <rajalakshmi.girish1@ibm.com>
057a0e2
to
81fccc1
Compare
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 that the role of this flag was to drive the behavior across many packages rather than pass it down to a specific package to not continue executing other tests in the package.
If I understand the tradeoff with this change is mostly about our pre-merge test:
- tests (if failing) will fail faster
- the results will be less complete (as they will contain only a single failed test, rather than all of them in a single package).
As we expect developer to rerun the whole target locally after fixing a test it sounds like a fair tradeoff.
Will this PR need an LGTM comment to merge? @jmhbnz Can you please help me understand what it needs to merge this one. |
or may be should all checks be successful? |
Two maintainer approvals are enough. |
Let's wait for test to pass. |
Will it auto merge if all tests pass and there are two approvals? |
Though there is mention of mode
fail_fast
, at https://github.com/etcd-io/etcd/blob/main/scripts/test_lib.sh#L252, and usages, there is no actual pass of-failfast
flag.Usages locations:
https://github.com/etcd-io/etcd/blob/main/scripts/test.sh#L163
https://github.com/etcd-io/etcd/blob/main/scripts/test.sh#L168