Skipping when fail-fast is set results in failure

[phisco]

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]

Cc @Fricounet as original author

[vladimirvivien]

Thanks for reporting this regression @phisco
cc @harshanarayana

[k8s-triage-robot]

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

[vladimirvivien]

/remove-lifecycle stale

[k8s-triage-robot]

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-triage-robot]

The Kubernetes project currently lacks enough active 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 rotten
• Close this issue with /close
• Offer to help out with Issue Triage

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

/lifecycle rotten