fix: exclude skipped tests from fail-fast#545
Conversation
Skipped tests (via t.Skip or skip-assessment regex) were incorrectly triggering fail-fast behavior because the shouldFailNow variable remained true when t.Skip() caused early function exit. Track skip state using defer to capture internalT.Skipped() after subtest completion, then exclude skipped assessments from the fail-fast condition.
k8s-ci-robot
added
the
sig/testing
|
Welcome @faganihajizada! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @faganihajizada. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
cncf-cla: no
vladimirvivien
left a comment
vladimirvivien
left a comment
There was a problem hiding this comment.
This looks good. Please any documentation update if behavior has changed.
Document that tests skipped via t.Skip() or --skip-assessment regex do not trigger fail-fast behavior, allowing subsequent assessments and features to continue executing normally.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: faganihajizada, vladimirvivien The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/test pull-e2e-framework-test |
|
@faganihajizada: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Skipped tests (via
t.Skip()or skip-assessment regex) were incorrectly triggering fail-fast behavior. This happened because whent.Skip()is called, it causes early function exit, leaving theshouldFailNowvariable set totrue.This PR tracks the skip state using
deferto captureinternalT.Skipped()after the subtest completes, then excludes skipped assessments from the fail-fast condition.Changes:
wasSkippedvariable to track if an assessment was skippeddeferto capture skip state (necessary becauset.Skip()exits early)!wasSkipped && (shouldFailNow || (e.cfg.FailFast() && t.Failed()))Which issue(s) this PR fixes:
Fixes #472
Special notes for your reviewer:
A previous attempt to fix this issue was made in #473 but was closed. This implementation takes a different approach:
finishedflag which had a flaw wheret.Fail()+ fail-fast wouldn't trigger properlyshouldFailNowlogic and adds a!wasSkippedguardDoes this PR introduce a user-facing change?
Fixed fail-fast mode incorrectly stopping test execution when assessments are skipped via t.Skip() or skip-assessment regex.
Additional documentation e.g., Usage docs, etc.:
N/A