fix(test): typed-feedback entry-return ordering assertion compared unrelated sites - #6840
Conversation
…related sites typed_feedback_trace_dump_runs_before_entry_return was red on main. Codegen is correct: add_pre_return_void_call splices the trace dump before every ret in main. The test compared rfind of the dump call against rfind of "ret i32 0" over the whole module. main has two returns, and the host-return early exit is the one that returns a literal i32 0, so the two searches landed on unrelated sites. Slice the IR to main's body and assert every ret is immediately preceded by the dump call. Deleting the add_pre_return_void_call in entry.rs now fails the test.
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe test now extracts ChangesTyped feedback validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
# Conflicts: # crates/perry-codegen/tests/typed_feedback.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-codegen/tests/typed_feedback.rs (1)
222-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep this regression check in a PR-visible unit-test path.
This remains an integration test under
crates/*/tests/*.rs. Per repository guidance, add equivalent acceptance coverage in acargo test-visible unit-test module so the regression is exercised by default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/tests/typed_feedback.rs` around lines 222 - 245, Move or duplicate this typed-feedback return-order regression check into a cargo test-visible unit-test module, while preserving coverage of every return in main via entry_fn_body and the existing assertions. Keep the integration test only if needed, but ensure the equivalent check runs by default through the repository’s unit-test path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/6840-typed-feedback-entry-return-ordering.md`:
- Around line 19-20: Update the CI explanation in the changelog entry to remove
the stale claim that crates/*/tests/ integration suites do not run per PR;
describe that changed suites are now selected by the PR e2e-scoped job, or
clearly frame the omission as historical context.
---
Nitpick comments:
In `@crates/perry-codegen/tests/typed_feedback.rs`:
- Around line 222-245: Move or duplicate this typed-feedback return-order
regression check into a cargo test-visible unit-test module, while preserving
coverage of every return in main via entry_fn_body and the existing assertions.
Keep the integration test only if needed, but ensure the equivalent check runs
by default through the repository’s unit-test path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39fc0c21-12ba-4ca1-b27e-d1f7d85cfbcc
📒 Files selected for processing (2)
changelog.d/6840-typed-feedback-entry-return-ordering.mdcrates/perry-codegen/tests/typed_feedback.rs
What
typed_feedback_trace_dump_runs_before_entry_returnincrates/perry-codegen/tests/typed_feedback.rshas been red onmain(reproduced at 8b2c736, macOS arm64).
Codegen is not the problem.
add_pre_return_void_callincrates/perry-codegen/src/codegen/entry.rs:469splicescall void @js_typed_feedback_maybe_dump_trace()in front of everyretinmain, via the return-site rewrite incrates/perry-codegen/src/function.rs:499.It still does.
The test was measuring the wrong thing. It compared two
rfinds over the wholemodule text:
mainhas more than one return. The emitted IR:The host-return early exit is the one that returns a literal
i32 0; theevent-loop exit returns the pending exit code. So
rfind("ret i32 0")landed onthe first return and
rfind(dump)on the second dump — two unrelated sites. Theassertion failed while the property it names held.
Change
Slice the IR to
main's body and check every return site: eachretmust beimmediately preceded by the dump call. That covers both returns instead of one,
and it is the ordering the epilogue actually guarantees.
Verification
cargo test -p perry-codegen --test typed_feedback— 15/15 pass.add_pre_return_void_callatentry.rs:469fails the new test. The old shape could not guarantee that.
Integration suites under
crates/*/tests/don't run per-PR (#5960), which is whythis sat red. Sibling stale test
native_proof_buffer_viewsis fixed in #6839;this one is untouched there.
Summary by CodeRabbit
Tests
Documentation