fix: replace cargo-expand with syn-based verification #611
Conversation
Remove slow cargo-expand dependency and replace with faster syn-based approach for testing Mergeable derive macro. Uses trait bounds to verify actual Merge implementation instead of macro expansion output. Fixes magicblock-labs#504 Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
WalkthroughRemoved workspace test tooling ( Changes
Sequence Diagram(s)sequenceDiagram
participant T as tests/test_merger.rs
participant G as merge_impl (codegen)
participant S as syn/quote/proc_macro2
participant P as pretty_print / prettyplease
participant A as assertions
Note over T,G: In-repo codegen replaces external cargo-expand fixtures
T->>G: provide input struct TokenStream
G->>S: parse & inspect input
S-->>G: produced impl TokenStream
G->>P: normalize generated impl
P-->>G: normalized tokens
G->>T: return normalized generated impl
T->>P: normalize expected impl
P-->>T: normalized expected tokens
T->>A: compare normalized tokens / assert results
A-->>T: pass / fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-14T09:56:14.047ZApplied to files:
🧬 Code graph analysis (1)magicblock-config-macro/tests/test_merger.rs (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(0 hunks)magicblock-config-macro/Cargo.toml(1 hunks)magicblock-config-macro/tests/test_merger.rs(2 hunks)
💤 Files with no reviewable changes (1)
- Cargo.toml
🔇 Additional comments (2)
magicblock-config-macro/tests/test_merger.rs (2)
3-4: LGTM!The new imports are necessary for the syn-based verification approach and are used appropriately in the new test.
89-136: Verification acceptable—fixture files exist and test coverage is adequate.All four referenced fixture files exist:
pass_merge.rs✓fail_merge_enum.rs✓fail_merge_union.rs✓fail_merge_unnamed.rs✓The test provides sufficient coverage across three verification layers:
- Compile-time verification (lines 132-135): trybuild ensures pass/fail fixtures behave as expected
- Trait implementation verification (line 86):
assert_merge::<TestConfig>()confirms the Merge trait is implemented- Behavioral verification (lines 22-72): Two behavioral tests execute actual merge operations with concrete assertions, catching any issues in the generated implementation logic
While the syn-based approach (lines 89-136) doesn't expose full macro expansion details like cargo-expand would, the behavioral tests verify the generated code's correctness at runtime. If the implementation structure was incorrect, the behavioral tests would fail. This trade-off between test speed and verification depth is acceptable for the stated performance goal.
| let has_mergeable_derive = file.items.iter().any(|item| { | ||
| if let Item::Struct(item_struct) = item { | ||
| item_struct | ||
| .attrs | ||
| .iter() | ||
| .any(|attr| attr.path().is_ident("derive")) | ||
| } else { | ||
| false | ||
| } | ||
| }); | ||
|
|
||
| assert!( | ||
| has_mergeable_derive, | ||
| "Expected struct with #[derive(Mergeable)]" | ||
| ); |
There was a problem hiding this comment.
Strengthen the derive attribute verification.
The current check only verifies that some struct has any derive attribute, not specifically #[derive(Mergeable)]. This check would pass for #[derive(Debug)] or any other derive macro, making it a very weak verification.
Consider parsing the derive attribute's tokens to verify Mergeable is actually present:
// Verify the file contains the Mergeable derive
let has_mergeable_derive = file.items.iter().any(|item| {
if let Item::Struct(item_struct) = item {
item_struct
.attrs
.iter()
- .any(|attr| attr.path().is_ident("derive"))
+ .any(|attr| {
+ attr.path().is_ident("derive") && {
+ attr.parse_args::<syn::Path>()
+ .map(|path| path.is_ident("Mergeable"))
+ .unwrap_or(false)
+ }
+ })
} else {
false
}
});Note: This simplified example assumes a single derive argument. For multiple derives like #[derive(Debug, Mergeable)], you'll need to parse syn::punctuated::Punctuated<syn::Path, syn::Token![,]> instead.
Committable suggestion skipped: line range outside the PR's diff.
|
hi @Dodecahedr0x, could you please review this? |
Dodecahedr0x
left a comment
Dodecahedr0x
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR!
We're expecting something along the line of what is done here: remove rust files containing the code and embed them directly in the test, parse them with syn and check that it matches what we expect.
Also, the github workflow for unit test installs cargo-expand, which should be removed.
Finally, please address code rabbit's comments
Remove cargo-expand dependency and replace trybuild fixture-based testing with embedded code verification using syn and token stream comparison Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/ci-test-unit.yml(0 hunks)magicblock-config-macro/Cargo.toml(0 hunks)magicblock-config-macro/tests/fixtures/fail_merge_enum.rs(0 hunks)magicblock-config-macro/tests/fixtures/fail_merge_enum.stderr(0 hunks)magicblock-config-macro/tests/fixtures/fail_merge_union.rs(0 hunks)magicblock-config-macro/tests/fixtures/fail_merge_union.stderr(0 hunks)magicblock-config-macro/tests/fixtures/fail_merge_unnamed.rs(0 hunks)magicblock-config-macro/tests/fixtures/fail_merge_unnamed.stderr(0 hunks)magicblock-config-macro/tests/fixtures/pass_merge.expanded.rs(0 hunks)magicblock-config-macro/tests/fixtures/pass_merge.rs(0 hunks)magicblock-config-macro/tests/test_merger.rs(2 hunks)
💤 Files with no reviewable changes (10)
- magicblock-config-macro/tests/fixtures/fail_merge_unnamed.rs
- magicblock-config-macro/Cargo.toml
- magicblock-config-macro/tests/fixtures/pass_merge.rs
- magicblock-config-macro/tests/fixtures/pass_merge.expanded.rs
- magicblock-config-macro/tests/fixtures/fail_merge_union.rs
- .github/workflows/ci-test-unit.yml
- magicblock-config-macro/tests/fixtures/fail_merge_enum.rs
- magicblock-config-macro/tests/fixtures/fail_merge_unnamed.stderr
- magicblock-config-macro/tests/fixtures/fail_merge_enum.stderr
- magicblock-config-macro/tests/fixtures/fail_merge_union.stderr
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-config-macro/tests/test_merger.rs (2)
magicblock-config-macro/src/merger.rs (1)
type_has_merge_method(8-23)magicblock-config-helpers/src/lib.rs (1)
merge(2-2)
🔇 Additional comments (2)
magicblock-config-macro/tests/test_merger.rs (2)
3-5: LGTM: Imports support the syn-based approach.The new imports (proc_macro2, quote, syn) are necessary for implementing the syn-based macro verification that replaces cargo-expand. All imports are utilized in the test helper functions below.
75-85: LGTM: Compile-time trait verification is effective.The trait bound check approach (
assert_merge<T: Merge>()) elegantly verifies at compile time that the Mergeable macro generates valid Merge implementations. This is complemented by the AST-based verification intest_merge_macro_codegen_verificationwhich validates correctness.
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-config-macro/tests/test_merger.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-config-macro/tests/test_merger.rs (3)
magicblock-config-macro/src/merger.rs (1)
type_has_merge_method(8-23)magicblock-config-helpers/src/lib.rs (1)
merge(2-2)magicblock-config/src/rpc.rs (1)
merge(29-36)
🔇 Additional comments (4)
magicblock-config-macro/tests/test_merger.rs (4)
1-5: LGTM! Appropriate dependencies for syn-based verification.The imports correctly bring in the necessary tools for the new token-stream-based macro testing approach, replacing the cargo-expand dependency.
22-73: LGTM! Comprehensive runtime behavior verification.The tests effectively verify both default-value replacement and non-default value preservation, ensuring the macro generates correct merge behavior.
75-85: LGTM! Effective compile-time trait verification.The trait bound check ensures the macro generates valid
Mergeimplementations. Combined with the runtime behavior tests, this provides comprehensive coverage.
87-168: The helper functions enable effective codegen verification.The
merge_impl,pretty_print, andassert_merge_impl_fnhelpers provide a solid foundation for comparing expected vs. generated implementations without cargo-expand. The approach is sound for testing proc macro output.Note: The
type_has_merge_methodduplication is already covered by past review comments.
|
hi @Dodecahedr0x, could you please check if my implementation is correct? also, should i add more test cases as codehabbit suggested? |
|
Yes, good job! Please handle all code rabbit's comments and we're good to merge |
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-config-macro/tests/test_merger.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-config-macro/tests/test_merger.rs (2)
magicblock-config-macro/src/merger.rs (1)
type_has_merge_method(8-23)magicblock-config-helpers/src/lib.rs (1)
merge(2-2)
🔇 Additional comments (4)
magicblock-config-macro/tests/test_merger.rs (4)
3-5: LGTM! Imports support the new syn-based testing approach.The addition of
proc_macro2,quote, andsynimports enables direct code generation and comparison without external fixture dependencies.
22-73: LGTM! Runtime tests provide solid coverage of merge behavior.These tests verify both the default-value-based merging for primitives and the nested merge delegation for Config types, covering the two main code paths in the generated implementation.
75-85: LGTM! Elegant compile-time verification approach.Using trait bounds to verify the
Mergeimplementation is an excellent replacement for cargo-expand. This catches compilation issues and ensures the trait is properly implemented without the overhead of external tooling.
297-334: Negative tests verify reference implementation panics, not macro compilation errors.These tests verify that
merge_implpanics with expected messages for invalid inputs (enum, tuple struct, unit struct). However, they don't verify the actualMergeablemacro produces compilation errors with helpful messages.Limitation:
- The real macro's error messages are controlled by
panic!calls in the macro implementation (likely insrc/lib.rs)- If the macro's error messages change or differ from
merge_impl, these tests won't catch it- Testing proc_macro error messages directly is challenging without tools like
trybuildCurrent coverage is acceptable:
- These tests document expected error behavior
- If the macro accepts invalid input that should be rejected, the user will discover it at compile time
- Runtime and compile-time tests provide the primary verification
This limitation is a trade-off of removing
trybuild, but acceptable given the test suite provides strong coverage overall.
|
Hey @Dodecahedr0x, Rust's proc-macro architecture doesn't allow us to directly inspect generated code at runtime, so we can't verify the actual macro output matches our reference implementation. We're using runtime tests to verify the macro works correctly and codegen tests to verify the logic is sound, but we can't guarantee 100% parity. This is a known limitation of proc-macro testing in Rust, the current approach catches regressions and verifies functionality, which is the best we can do with the architectural constraints. |
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-config-macro/tests/test_merger.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-config-macro/tests/test_merger.rs (3)
magicblock-config-macro/src/merger.rs (1)
type_has_merge_method(8-23)magicblock-config-helpers/src/lib.rs (1)
merge(2-2)magicblock-config/src/rpc.rs (1)
merge(29-36)
🔇 Additional comments (2)
magicblock-config-macro/tests/test_merger.rs (2)
3-5: LGTM: Appropriate imports for syn-based verification.The added imports support the new test infrastructure correctly.
75-85: LGTM: Strong compile-time verification.This trait bound check directly verifies the actual
Mergeablemacro generates valid code. Combined with the runtime tests, this provides robust verification of macro correctness.
| /// Helper function that compares generated merge impl with expected output | ||
| fn assert_merge_impl_fn(code: TokenStream2, expected: TokenStream2) { | ||
| let generated = merge_impl(code); | ||
|
|
||
| assert_eq!( | ||
| pretty_print(generated), | ||
| pretty_print(expected), | ||
| "Generated merge implementation does not match expected output" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Helper tests reference implementation, not actual macro.
While this function is correctly implemented, it compares merge_impl output against expectations. It doesn't verify what the actual Mergeable derive macro generates, creating a verification gap.
If you remove the codegen tests (recommended in comment on lines 87-154), delete this helper as well.
🤖 Prompt for AI Agents
In magicblock-config-macro/tests/test_merger.rs around lines 167-176, the helper
assert_merge_impl_fn compares the hand-called merge_impl function instead of the
actual Mergeable derive macro output, leaving a verification gap; if you are
removing the codegen tests as suggested, delete this helper entirely and any
callers, otherwise change the helper to expand the real derive macro (e.g.,
invoke the proc-macro expansion/path used by your test harness or use trybuild
to compile a test crate and capture the expanded impl) and compare that
expansion to the expected TokenStream rather than calling merge_impl directly.
|
hi @Dodecahedr0x , there was a lint issue, I’ve pushed a fix. also noticed a deployment issue, but it seems to be an authorization problem since I don’t have permission to trigger it. |
|
❌ Failed to trigger deploy for branch ``. Make sure you have permission or use the manual deploy link in the PR comment. |
|
hi @Dodecahedr0x, can you please trigger the deployment? I don’t have permission to do it. all test cases have passed. |
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
|
I think the test should use the actual |
2 participants
Remove slow cargo-expand dependency and replace with faster syn-based
approach for testing Mergeable derive macro. Uses trait bounds to verify
actual Merge implementation instead of macro expansion output.
Fixes #504
Summary by CodeRabbit
Tests
Chores