Skip to content

kola/harness: fix rerun success logic #4559

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

Open
nikita-dubrovskii wants to merge 1 commit into
base: main
Choose a base branch
from
Open

kola/harness: fix rerun success logic #4559

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions mantle/kola/harness.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,13 +872,27 @@ func runProvidedTests(testsBank map[string]*register.Test, patterns []string, mu
newOutputDir := filepath.Join(outputDir, "rerun")
fmt.Printf("\n\n======== Re-running failed tests (flake detection) ========\n\n")
reRunErr := runProvidedTests(testsToRerun, []string{"*"}, multiply, false, rerunSuccessTags, pltfrm, newOutputDir)
if reRunErr == nil && allTestsAllowRerunSuccess(testsToRerun, rerunSuccessTags) {
// Four possible outcomes from the rerun:
// 1. reRunErr == nil && allTestsAllowRerunSuccess == true:
// Tests passed on rerun AND have the required tags to allow rerun success.
// Reset to success (runErr = nil).
// 2. reRunErr != nil && allTestsAllowRerunSuccess == false:
// Tests failed on rerun and don't have tags. Use the rerun error (runErr = reRunErr).
// 3. reRunErr == nil && allTestsAllowRerunSuccess == false:
// Tests passed on rerun BUT don't have the required tags to allow rerun success.
// Keep the original error (don't modify runErr) - the initial failure stands.
// 4. reRunErr != nil && allTestsAllowRerunSuccess == true:
// Tests failed on rerun even though they have allow-rerun-success tags.
// Use the rerun error (runErr = reRunErr) - tags only matter when rerun passes.
if reRunErr != nil {
// Scenarios 2 & 4: Rerun failed - use the rerun error regardless of tags
runErr = reRunErr
} else if allTestsAllowRerunSuccess(testsToRerun, rerunSuccessTags) {
// Scenario 1: Rerun passed AND tests have tags to allow rerun success
runErr = nil // reset to success since all tests allowed rerun success
numFailedTests = 0 // zero out the tally of failed tests
} else {
runErr = reRunErr
}

// else: Scenario 3 - keep original runErr (test passed on rerun but doesn't allow rerun success)
Comment on lines +887 to +895
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When Scenario 3 occurs (tests pass on rerun but lack the necessary tags), the output might be confusing to users because they will see a PASS for the rerun followed by an overall FAIL for the test suite. Adding an explicit log message here would clarify why the original failure is being preserved despite the successful rerun.

		if reRunErr != nil {
			// Scenarios 2 & 4: Rerun failed - use the rerun error regardless of tags
			runErr = reRunErr
		} else if allTestsAllowRerunSuccess(testsToRerun, rerunSuccessTags) {
			// Scenario 1: Rerun passed AND tests have tags to allow rerun success
			runErr = nil       // reset to success since all tests allowed rerun success
			numFailedTests = 0 // zero out the tally of failed tests
		} else {
			// Scenario 3: Rerun passed but tags don't allow success.
			fmt.Printf("Tests passed on rerun but lack tags to allow rerun success; original failure stands.\n")
		}

Copy link
Copy Markdown
Member

@jbtrystram jbtrystram May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want this behaviour ? That feels like a waste to throw an error if a rerun worked.
This scenario would likely just end up in someone notice it and go add the rerun tag.

Copy link
Copy Markdown
Contributor Author

@nikita-dubrovskii nikita-dubrovskii May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how this feature is supposed to work: return SUCCESS only when the tagged test(s) pass on rerun. When this feature was added, mostly the needs-internet tests were flaky, so this tag is hardcoded in the pipeline. But we do rerun any failed test, as far as I remember, to gather some statistics on which other tests and tags are also flaky. If they succeed on rerun, we would otherwise not be notified about failures from the first run.

}

// Return ErrWarnOnTestFail when ONLY tests with warn:true feature failed
Expand Down