test(pybind): drop shadow-name guard, cover @= identity (follow-up to #986) - #1009
Conversation
Follow-up to the merged #986, addressing review feedback on pytests/binding_inplace_test.py: - Remove test_shadow_bindings_are_gone (@IvanaGyro). It only asserts that the deleted c__*/c_* shadow names are absent -- a one-time migration guard with no value for future code, and the last place those dead names were referenced. - Add test_imatmul_preserves_identity (@pcchen). `@=` is the one genuine behavioral change from #986: the old wrapper was misspelled `def __imatmul` (no trailing `__`), so `a @= b` fell back to `a = a @ b` and rebound `a` to a new object. With __imatmul__ bound correctly it now mutates in place and preserves identity; this was previously untested. Verified against a fresh build: the new test passes on the post-#986 module (real __imatmul__) and fails on the pre-#986 module (fallback rebinds a), so it genuinely pins the fixed behavior. Full file: 7 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request removes the test_shadow_bindings_are_gone test case and adds a new test test_imatmul_preserves_identity to verify that the @= operator correctly performs in-place matrix multiplication and preserves object identity. There are no review comments, so we have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1009 +/- ##
=======================================
Coverage 31.82% 31.82%
=======================================
Files 230 230
Lines 33124 33124
Branches 13852 13852
=======================================
Hits 10543 10543
Misses 15539 15539
Partials 7042 7042
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code ReviewClean, correct test-only follow-up to #986 that closes both review comments on
No findings — a strict improvement to the test suite. Posted by Claude Code on behalf of @pcchen |
pcchen
left a comment
There was a problem hiding this comment.
Approving. Correct, non-vacuous @= identity test (snapshots Dot(a,b) before the in-place op, asserts identity + zero-norm correctness, and per the PR notes fails on the pre-#986 module), reasonable removal of the one-time shadow-name migration guard, no orphaned imports, and full test CI (BuildAndTest Linux+macOS, pytest, wheels, codecov) green on the current commit. Closes both #986 review comments.
Posted by Claude Code on behalf of @pcchen
Follow-up to the merged #986, addressing the two review comments on
pytests/binding_inplace_test.py.Changes
Remove
test_shadow_bindings_are_gone(per @IvanaGyro, review comment: "This test is useless for the future code. This should be clean up."). It only asserts that the deletedc__*/c_*shadow names are absent — a one-time migration guard with no value for future code, and the last place in the tree that even referenced those dead names.Add
test_imatmul_preserves_identity(per @pcchen's approval follow-up).@=is the one genuine behavioral change from refactor(pybind): bind in-place methods directly, drop the c__* shadow API #986: on master the wrapper was misspelleddef __imatmul(missing the trailing__), soa @= bfell back toa = a @ band reboundato a new object. With__imatmul__bound correctly it now mutates in place and preserves identity. This path was previously untested.Verification
Built the Python module from the post-#986 tree and ran the file:
__imatmul__present,a @= bmutates in place,a is alias, and the alias observes the result ofa @ b).a @= brebindsa,a is aliasis False) — confirming it genuinely pins the fixed behavior rather than passing vacuously.Net: −29/+15 (one dead test out, one meaningful test in).
🤖 Generated with Claude Code