Skip to content

[None][fix] test_cbts_coverage_pilot: use capfd to avoid autouse-fixture clash - #18076

Merged
lowsfer merged 3 commits into
NVIDIA:mainfrom
chang-l:fix/cbts-capsys-capfd-clash
Aug 21, 2026
Merged

[None][fix] test_cbts_coverage_pilot: use capfd to avoid autouse-fixture clash#18076
lowsfer merged 3 commits into
NVIDIA:mainfrom
chang-l:fix/cbts-capsys-capfd-clash

Conversation

@chang-l

@chang-l chang-l commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Description

tests/unittest/scripts/test_cbts_coverage_pilot.py::test_main_reads_bot_trigger_payload (added in #17996) fails deterministically on setup in every L0 unittest/scripts shard:

E       cannot use capsys and capfd at the same time

Root cause: tests/unittest/conftest.py defines the autouse fixture cuda_error_early_quit(capfd), so every test under tests/unittest already holds capfd; a test requesting capsys is therefore rejected by pytest at setup.

Fix: switch the test to capfd (fd-level capture is a superset of capsys; readouterr() is API-identical). Two-line change.

Evidence: L0_MergeRequest_PR builds 55686 and 55711 on PR #17693 (A10-PyTorch-3, unittest/scripts shard). Also reported on #17996.

Test Coverage

The fixed test itself: tests/unittest/scripts/test_cbts_coverage_pilot.py.

PR Checklist

  • PR title and description convey the end-user impact
  • Test coverage: the change is itself a test fix
  • DCO sign-off in commit

🤖 Generated with Claude Code

Dev Engineer Review

  • Replaced capsys with capfd in test_main_reads_bot_trigger_payload.
  • The change matches the autouse cuda_error_early_quit(capfd) fixture.
  • Removed the related waiver entry from tests/integration/test_lists/waives.txt.
  • The waiver entry used a valid test path and had no unintended scope changes.
  • No performance, error-handling, or API consistency issues found.

QA Engineer Review

  • Modified test: test_main_reads_bot_trigger_payload.
  • Removed its entry from tests/integration/test_lists/waives.txt.
  • The test is covered by the CI test list through the waiver removal.
  • Verdict: sufficient.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 877fc976-6c16-4e75-a80c-44b9ee3340e6

📥 Commits

Reviewing files that changed from the base of the PR and between 3142214 and 0bab485.

📒 Files selected for processing (1)
  • tests/integration/test_lists/waives.txt

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The coverage pilot test now uses pytest’s capfd fixture to capture file-descriptor output. Its output assertions remain unchanged. The related waiver entry was removed.

Changes

Coverage test update

Layer / File(s) Summary
Capture subprocess output
tests/unittest/scripts/test_cbts_coverage_pilot.py, tests/integration/test_lists/waives.txt
The test changes its fixture parameter and captured-output read from capsys to capfd. The related waiver entry is removed.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 0bab4

This is a localized test-only fixture update, and no actionable merge-blocking risk remains beyond normal validation.

Suggested reviewers: bowenfu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the repository format and clearly describes the switch to capfd to fix the fixture conflict.
Description check ✅ Passed The description explains the failure, root cause, fix, evidence, test coverage, and relevant checklist items.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@lowsfer

lowsfer commented Aug 21, 2026

Copy link
Copy Markdown
Member

/bot run --disable-fail-fast

@lowsfer
lowsfer enabled auto-merge (squash) August 21, 2026 17:03
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68352 [ run ] triggered by Bot. Commit: 3142214 Link to invocation

…ure clash

tests/unittest/conftest.py has an autouse fixture cuda_error_early_quit(capfd),
so any test under tests/unittest requesting capsys errors on setup with
'cannot use capsys and capfd at the same time'. Switch
test_main_reads_bot_trigger_payload to capfd (fd-level capture is a superset
and readouterr() is API-identical).

Signed-off-by: Chang Liu <liuc@nvidia.com>
@chang-l
chang-l force-pushed the fix/cbts-capsys-capfd-clash branch from 7220e0a to 460d3a1 Compare August 21, 2026 17:39
@chang-l

chang-l commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68359 [ run ] triggered by Bot. Commit: 460d3a1 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68352 [ run ] completed with state ABORTED. Commit: 3142214
/LLM/main/L0_MergeRequest_PR pipeline #55775 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lowsfer

lowsfer commented Aug 21, 2026

Copy link
Copy Markdown
Member

FYI, there is a waiver for the issue now, so maybe it's a good idea to unwaive it with this PR. The bug is 6647594

chang-l and others added 2 commits August 21, 2026 10:55
The waiver (https://nvbugs/6647594, added in NVIDIA#18080) is no longer needed:
the capsys/capfd autouse-fixture clash is fixed in this PR by switching the
test to capfd. Remove the SKIP entry so the test runs again.

Signed-off-by: Chang Liu <liuc@nvidia.com>
@chang-l

chang-l commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Done — updated the branch past #18080 and removed the waiver line in this PR (0bab485), so the fix and the unwaive land atomically and this PR's CI validates the test actually runs and passes. Kept PR #17693 away from waives.txt entirely — its branch predates the waiver and already passes via the same capfd fix, and it will drop the test file from its diff after this merges.

@chang-l

chang-l commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68362 [ run ] triggered by Bot. Commit: 0bab485 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68359 [ run ] completed with state ABORTED. Commit: 460d3a1

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68362 [ run ] completed with state SUCCESS. Commit: 0bab485
/LLM/main/L0_MergeRequest_PR pipeline #55789 completed with status: 'SUCCESS'

CI Report

Link to invocation

@lowsfer
lowsfer merged commit 4153606 into NVIDIA:main Aug 21, 2026
10 checks passed
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.

4 participants