Skip to content

Feat: Add Note for tests with known Issues (flakiness) - #22

Open
jbsmith7741 wants to merge 1 commit into
mainfrom
trackIssues
Open

Feat: Add Note for tests with known Issues (flakiness)#22
jbsmith7741 wants to merge 1 commit into
mainfrom
trackIssues

Conversation

@jbsmith7741

Copy link
Copy Markdown
Member

No description provided.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add KnownIssue() annotation for flaky tests (visibility-only)

✨ Enhancement 📝 Documentation 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Add KnownIssue(reason) to annotate failing cases as known flaky issues.
• Append a yellow "known issue" suffix to failure output without changing pass/fail.
• Document and demonstrate usage, and add unit coverage for message behavior.
Diagram

graph TD
  A["Test author"] --> B["trial.New(...)"] --> C["KnownIssue(reason)"] --> D["SubTest(t)"] --> E["testCase()"] --> F["Result message"] --> G["Go test output"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Per-case KnownIssue metadata (on Case struct)
  • ➕ More granular: only specific flaky cases get annotated
  • ➕ Avoids trial-wide annotation leaking onto unrelated failures
  • ➖ API surface expands (new Case field or builder per case)
  • ➖ Requires updating case declarations and docs more broadly
2. Integrate with testing.TB as Skip/Fail wrapper (e.g., Skip with reason)
  • ➕ Can reduce CI noise by skipping known flaky cases
  • ➕ Clear signal in test output via standard Go test semantics
  • ➖ Changes pass/fail semantics (explicitly not desired here)
  • ➖ Risk of masking real regressions behind skips

Recommendation: Keep the current visibility-only, trial-wide KnownIssue(reason) approach: it is low-risk, additive, and matches the stated goal (annotate failures without changing outcomes). If teams later need finer control, consider adding an optional per-case known-issue field as an extension rather than changing this behavior.

Files changed (4) +69 / -5

Enhancement (1) +25 / -5
trial.goAdd KnownIssue() support and append reason to failing messages +25/-5

Add KnownIssue() support and append reason to failing messages

• Adds yellow coloring helper and stores a trial-level known-issue reason. On failure, appends a '(known issue: ...)' suffix to the result message without changing success evaluation.

trial.go

Tests (1) +23 / -0
trial_test.goAdd unit test asserting KnownIssue suffix appears only on failures +23/-0

Add unit test asserting KnownIssue suffix appears only on failures

• Adds 'TestKnownIssue' to verify failing cases include the known-issue reason and passing cases do not. Uses 'testCase' directly to validate message formatting behavior.

trial_test.go

Documentation (2) +21 / -0
api.mdDocument KnownIssue() in fluent Trial API list +1/-0

Document KnownIssue() in fluent Trial API list

• Adds 'KnownIssue(reason)' to the API table to advertise the new fluent configuration method for known flaky tests.

docs/api.md

examples.mdAdd KnownIssue visibility example and expected output +20/-0

Add KnownIssue visibility example and expected output

• Introduces a new section demonstrating 'KnownIssue()' usage and clarifies that it is visibility-only. Shows the expected failure suffix format and notes passing cases are unchanged.

docs/examples.md

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. KnownIssue skipped on timeout 🐞 Bug ≡ Correctness
Description
Trial.testCase appends the known-issue suffix only at the end of the function, but timeout and
panic-related code paths return early, so those failing cases never include the KnownIssue
annotation.
This contradicts the documented behavior (“When a case fails, output includes a suffix…”) and
reduces visibility for some of the most important failure modes (timeouts/panics).
Code

trial.go[R240-242]

+	if !result.Success && t.knownIssueReason != "" {
+		result.Message += colorYellow(fmt.Sprintf(" (known issue: %s)", t.knownIssueReason))
+	}
Evidence
testCase() returns early on timeout and when panicCheck is set, before the code that appends the
known-issue suffix. The documentation section states that failures include the known-issue suffix,
which is not true for these early-return failures.

trial.go[191-244]
trial.go[138-144]
docs/examples.md[367-383]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`KnownIssue()` is intended to append a suffix to *any failing case*, but `testCase()` currently appends the suffix only at the end of the normal path. Early returns (timeout, panic-handling) bypass this, so those failing results lack the known-issue annotation.

## Issue Context
- Timeout failures (`ctx.Done()`) and panic-related failures (`panicCheck`) return before reaching the KnownIssue append block.
- Docs explicitly state that when a case fails, output includes the known-issue suffix.

## Fix Focus Areas
- trial.go[191-244]

### Implementation direction
- Centralize the KnownIssue decoration in a helper (e.g., `t.decorateResult(r *result)` that appends when `!r.Success && t.knownIssueReason != ""`) and call it immediately before *every* return in `testCase()`.
- Alternatively, refactor `testCase()` to a single exit path where the decoration always runs before returning.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread trial.go
Comment on lines +240 to +242
if !result.Success && t.knownIssueReason != "" {
result.Message += colorYellow(fmt.Sprintf(" (known issue: %s)", t.knownIssueReason))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Knownissue skipped on timeout 🐞 Bug ≡ Correctness

Trial.testCase appends the known-issue suffix only at the end of the function, but timeout and
panic-related code paths return early, so those failing cases never include the KnownIssue
annotation.
This contradicts the documented behavior (“When a case fails, output includes a suffix…”) and
reduces visibility for some of the most important failure modes (timeouts/panics).
Agent Prompt
## Issue description
`KnownIssue()` is intended to append a suffix to *any failing case*, but `testCase()` currently appends the suffix only at the end of the normal path. Early returns (timeout, panic-handling) bypass this, so those failing results lack the known-issue annotation.

## Issue Context
- Timeout failures (`ctx.Done()`) and panic-related failures (`panicCheck`) return before reaching the KnownIssue append block.
- Docs explicitly state that when a case fails, output includes the known-issue suffix.

## Fix Focus Areas
- trial.go[191-244]

### Implementation direction
- Centralize the KnownIssue decoration in a helper (e.g., `t.decorateResult(r *result)` that appends when `!r.Success && t.knownIssueReason != ""`) and call it immediately before *every* return in `testCase()`.
- Alternatively, refactor `testCase()` to a single exit path where the decoration always runs before returning.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant