Repository navigation
Fix: report failing SceneTest case context - #1927
Merged
ChaoZheng109 merged 1 commit intoAug 24, 2026
Merged
Conversation
Wrap each case execution failure with its class-qualified case name while preserving the original exception and its message. Keep device error markers visible so pooled Worker poison detection still recycles failed lanes. Refs hw-native-sys#1832
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesScene case error reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change is localized to failure reporting and regression coverage, with the supplied checks passing; no actionable merge-blocking risk remains beyond normal review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ChaoZheng109
approved these changes
Aug 24, 2026
This was referenced Aug 25, 2026
ChaoWao
added a commit
that referenced
this pull request
Aug 26, 2026
`run_class_cases` reported which case failed by wrapping every case exception in
a fresh `RuntimeError`. That makes a negative scene test — one whose subject is
the exception its own orchestration raises — unable to pass:
with pytest.raises(ValueError, match="ibing mode is only supported for nranks=2"):
super().test_run(st_platform, st_worker, request)
`TestAllreduceIbingNranksError` asserts exactly that. The `ValueError` does fire,
in `allreduce_orch_fn`, and reaches the test as a `RuntimeError` carrying it as
`__cause__`, which `pytest.raises(ValueError)` does not match. The wrapper arrived
in #1927 and did not update this test; the case is `manual: True`, and per-PR CI
runs the sim and NPU workflows with their default `manual_mode: exclude`, so only
the daily full sweep reaches it. It has been red there since.
The case name is now added to the exception already in flight —
`_name_failing_case` rewrites `args[0]` — and the original is re-raised with a
bare `raise`. Type, identity, traceback and attributes all survive, and `str()`
still ends with the original text.
Two things depend on that:
- a negative test can assert on the type its orchestration raised, which is the
only way to state what such a test is actually checking;
- `conftest._requires_l2_worker_retirement` gates on
`issubclass(excinfo.type, RuntimeError)` before matching device-poison codes.
Every code in `_DEVICE_POISON_CODES` and every string in
`_L2_WORKER_RETIREMENT_MARKERS` names a native-layer failure
(`{prepare,launch,poll,finalize}_native_run`, `simpler_init`, device
quarantine), all of which surface as `RuntimeError`, so keeping the type keeps
them classifiable. The wrapper had been widening non-RuntimeError failures into
that gate, never narrowing.
`TestAllreduceIbingNranksError` needs no change and is the regression test: it
fails on the parent commit with `DID NOT RAISE <class 'RuntimeError'>` and passes
here. The unit contract test moves to the new shape — same type out, case name in
the message, no wrapper to unwrap — and gains a case for an exception raised with
no arguments, so the annotation cannot degrade to a bare `"…::case: "`.
`tests/st/worker/collectives/allreduce/test_allreduce.py:326` is the only place
that wraps `super().test_run(...)` in `pytest.raises`; the other overrides call it
plainly, so no other scene test changes behaviour.
Verified: `pytest tests/ut/py` 1937 passed / 14 skipped, the whole
`tests/st/worker/collectives` suite green in daily mode (`--manual include`), and
the per-PR sim gate (`--manual exclude`) green on a2a3sim and a5sim.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
python -m pytest tests/ut/py/test_scene_test_cli_contract.py -q(8 passed)tests/utreached 1667 passed and 19 skipped; the unrelatedtest_second_child_failure_reaps_firstsuite-load teardown flake failed and passed when rerun aloneCloses #1832