Skip to content

Skipping when fail-fast is set results in failure #472

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.

Sign up for GitHub

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

Jump to bottom
Open
phisco opened this issue Jan 3, 2025 · 3 comments · May be fixed by #473
Open

Skipping when fail-fast is set results in failure #472

phisco opened this issue Jan 3, 2025 · 3 comments · May be fixed by #473
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@phisco
Copy link
Contributor

phisco commented Jan 3, 2025

What happened?

See reproducer here.

Running the test defined results in the whole suite failing in the latest releases, both 0.4.0 and 0.5.0 on main branch:

go test -v -test.count 1 ./...
=== RUN   TestSkip
=== RUN   TestSkip/skip
=== RUN   TestSkip/skip/Assess_1
    main_test.go:23: Assess 1 (should be printed)
    main_test.go:24: skipping Assess 1
--- FAIL: TestSkip (0.00s)
    --- FAIL: TestSkip/skip (0.00s)
        --- SKIP: TestSkip/skip/Assess_1 (0.00s)
FAIL
FAIL    github.com/phisco/e2e-framework-test-skip       0.497s
FAIL

What did you expect to happen?

Just the assessment Assess_1 to be skipped, but the suite to succeed as in 0.3.0, see PR .

go test -v -test.count 1 ./...
=== RUN   TestSkip
=== RUN   TestSkip/skip
=== RUN   TestSkip/skip/Assess_1
    main_test.go:23: Assess 1 (should be printed)
    main_test.go:24: skipping Assess 1
=== RUN   TestSkip/skip/Assess_2
    main_test.go:28: Assess 2 (should be printed)
=== RUN   TestSkip/succeed
=== RUN   TestSkip/succeed/Assess_1
    main_test.go:35: Assess 1 (should be printed)
=== RUN   TestSkip/succeed/Assess_2
    main_test.go:39: Assess 2 (should be printed)
--- PASS: TestSkip (0.00s)
    --- PASS: TestSkip/skip (0.00s)
        --- SKIP: TestSkip/skip/Assess_1 (0.00s)
        --- PASS: TestSkip/skip/Assess_2 (0.00s)
    --- PASS: TestSkip/succeed (0.00s)
        --- PASS: TestSkip/succeed/Assess_1 (0.00s)
        --- PASS: TestSkip/succeed/Assess_2 (0.00s)
PASS
ok      github.com/phisco/e2e-framework-test-skip       0.467s

How can we reproduce it (as minimally and precisely as possible)?

See reproducer here.

Anything elese we need to know?

I think the issue was introduced by #391, as a t.Skip will leave shouldFailNow set to true exactly as a t.Fail here:

e2e-framework/pkg/env/env.go

Lines 506 to 530 in 72eb7e1

// shouldFailNow catches whether t.FailNow() is called in the assessment.
// If it is, we won't proceed with the next assessment.
var shouldFailNow bool
newT.Run(assessName, func(internalT *testing.T) {
internalT.Helper()
skipped, message := e.requireAssessmentProcessing(assess, i+1)
if skipped {
internalT.Skip(message)
}
// Set shouldFailNow to true before actually running the assessment, because if the assessment
// calls t.FailNow(), the function will be abruptly stopped in the middle of `e.executeSteps()`.
shouldFailNow = true
ctx = e.executeSteps(ctx, internalT, []types.Step{assess})
// If we reach this point, it means the assessment did not call t.FailNow().
shouldFailNow = false
})
// Check if the Test assessment under question performed either 2 things:
// - a t.FailNow() invocation
// - a `t.Fail()` or `t.Failed()` invocation
// In one of those cases, we need to track that and stop the next set of assessment in the feature
// under test from getting executed.
if shouldFailNow || (e.cfg.FailFast() && newT.Failed()) {
failed = true
break
}

E2E Provider Used

kind

e2e-framework Version

0.5.0

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
@phisco phisco added the kind/bug Categorizes issue or PR as related to a bug. label Jan 3, 2025
@phisco
Copy link
Contributor Author

phisco commented Jan 3, 2025

Cc @Fricounet as original author

@phisco phisco linked a pull request Jan 3, 2025 that will close this issue
Open
@vladimirvivien
Copy link
Contributor

Thanks for reporting this regression @phisco
cc @harshanarayana

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: properly handle skip on assessments phisco/e2e-framework
4 participants
@vladimirvivien @phisco @k8s-ci-robot @k8s-triage-robot