Skip to content

Conversation

@tonyredondo
Copy link
Member

@tonyredondo tonyredondo commented Jan 26, 2026

What does this PR do?

JIRA: SDTEST-2986

Adds a new test.final_status tag to CI Visibility test spans that indicates the final adjusted status for a test after considering all executions (including retries). This helps distinguish between the raw test outcome and the final computed result.

The priority order for determining final status is:

  1. Quarantined/Disabledskip (management directives take precedence)
  2. Any pass in retriespass (a single pass means the test ultimately passed)
  3. Any fail (no pass)fail (all executions failed)
  4. Current is skipskip (single-execution skip)
  5. Defaultfail

Key Changes

  • New constant: TestFinalStatus = "test.final_status" in constants/test_tags.go
  • New file: final_status.go with helper functions:
    • calculateFinalStatus() - computes the final status based on execution outcomes
    • isFinalExecution() - determines if current execution is the last one (no more retries)
    • computeAdjustedRetryCount() - mirrors retry count logic for prediction
    • willRetryAfterExecution() - predicts if another retry will happen
  • Metadata tracking: Added anyExecutionPassed, anyExecutionFailed, remainingRetries, isEfdInParallel, and skipReason fields to testExecutionMetadata
  • Flag propagation: isEfdInParallel and hasAdditionalFeatureWrapper are propagated to subtests
  • Skip reason preservation: Store skip reason when instrumentCloseAndSkip returns early, use it in defer block
  • Parallel EFD handling: Option A - don't set test.final_status for parallel EFD (impossible to determine final execution)
  • ATF precedence: ATF tests compute test.final_status even when parallel EFD is enabled
  • Test coverage: Added assertions in testcontroller_test.go and subtestcontroller_test.go

Motivation

The test.final_status tag allows CI Visibility consumers to understand the ultimate outcome of a test after all retries and management directives have been applied. This is particularly useful for:

  • Flaky test detection (distinguishing flaky passes from solid passes)
  • Test management reporting (showing adjusted status for quarantined/disabled tests)
  • Retry analysis (understanding if retries helped the test pass)

Test Plan

  • All existing test scenarios pass (7 scenarios in testcontroller_test.go)
  • All subtest scenarios pass (10 scenarios in subtestcontroller_test.go)
  • Added test.final_status assertions for:
    • Pass case (single execution and after retries)
    • Skip case (single execution, disabled, quarantined)
    • Parallel EFD (Option A - no final_status set)
    • ITR-skipped tests
    • Test management tests (disabled/quarantined with ATF)
    • Subtests (baseline, disabled, quarantined, parent quarantined)

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@tonyredondo tonyredondo requested a review from a team as a code owner January 26, 2026 18:02
@tonyredondo tonyredondo marked this pull request as draft January 26, 2026 18:02
@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

❌ Patch coverage is 72.43590% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.15%. Comparing base (fa72b69) to head (e2addd8).

Files with missing lines Patch % Lines
...egrations/gotesting/instrumentation_orchestrion.go 33.33% 27 Missing and 3 partials ⚠️
...ivisibility/integrations/gotesting/final_status.go 81.66% 8 Missing and 3 partials ⚠️
...nal/civisibility/integrations/gotesting/testing.go 94.73% 0 Missing and 2 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
...sibility/integrations/gotesting/instrumentation.go 82.43% <100.00%> (+0.55%) ⬆️
...nal/civisibility/integrations/gotesting/testing.go 59.38% <94.73%> (+2.86%) ⬆️
...ivisibility/integrations/gotesting/final_status.go 81.66% <81.66%> (ø)
...egrations/gotesting/instrumentation_orchestrion.go 29.93% <33.33%> (+0.66%) ⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter
Copy link

pr-commenter bot commented Jan 26, 2026

Benchmarks

Benchmark execution time: 2026-01-27 15:02:19

Comparing candidate commit e2addd8 in PR branch tony/civiz-test-final-status with baseline commit fa72b69 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 157 metrics, 7 unstable metrics.

@tonyredondo tonyredondo force-pushed the tony/civiz-test-final-status branch from e730220 to de04d2d Compare January 26, 2026 21:41
This adds a new `test.final_status` tag that indicates the final adjusted status
for a test after considering all executions (retries). The priority order is:
- Quarantined/Disabled -> skip
- Any pass in retries -> pass
- Any fail (no pass) -> fail
- Current is skip -> skip
- Default -> fail

Key changes:
- Add TestFinalStatus constant and calculateFinalStatus() helper
- Add isFinalExecution() to determine when to set the tag
- Track anyExecutionPassed/anyExecutionFailed across retries
- Propagate isEfdInParallel flag to subtests for parallel EFD (Option A)
- Set test.final_status in instrumentCloseAndSkip/instrumentSkipNow for non-wrapper tests
- ATF takes precedence over parallel EFD for final status computation
- Preserve skip reason through skipReason field for wrapper tests
- Add test assertions for final_status in testcontroller_test.go and subtestcontroller_test.go
@tonyredondo tonyredondo force-pushed the tony/civiz-test-final-status branch from de04d2d to a3dfbb1 Compare January 27, 2026 10:05
@tonyredondo tonyredondo marked this pull request as ready for review January 27, 2026 10:05
return constants.TestStatusSkip
}
if anyPassed {
return constants.TestStatusPass
Copy link
Member

@anmarchenko anmarchenko Jan 27, 2026

Choose a reason for hiding this comment

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

this would not be right for the attempt to fix flow:

If the test is in attempt-to-fix flow, it should be reported to the test framework as failed if any of the retries failed - so flaky test that is still flaky in attempt-to-fix fails the build.

I was recently fixing this issue for the Ruby library.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, great, changed in e2addd8

For Attempt-To-Fix (ATF) tests, the final_status should be 'fail' if any
execution failed, even if some passed. This ensures flaky tests that are
still flaky in ATF mode fail the build, correctly verifying that a fix
actually resolves the flakiness.

Updated calculateFinalStatus() priority order:
1. quarantined/disabled -> skip (unchanged)
2. ATF && anyFailed -> fail (NEW)
3. anyPassed -> pass
4. anyFailed -> fail
5. currentIsSkip -> skip
6. default -> fail
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.

3 participants