501: feat: Use macrotest to test the expanded macro code#62
501: feat: Use macrotest to test the expanded macro code#62martin-augment wants to merge 6 commits intomainfrom
macrotest to test the expanded macro code#62Conversation
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
WalkthroughThe pull request enhances the Avro schema derivation macro by improving code generation quality through fully-qualified type paths (e.g., ✨ Finishing Touches
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure for the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Review: Use macrotest to test the expanded macro codeOverall this is a well-structured improvement. Replacing brittle raw-string token comparisons with Positives
Concerns1. Expanded snapshots contain nightly-internal APIs (high risk of spurious CI failures) The symbols: ::alloc::boxed::box_assume_init_into_vec_unsafe(
::alloc::intrinsics::write_box_via_move(
::alloc::boxed::Box::new_uninit(),
[...],
),
),This is the nightly-specific expansion of Suggestion: Pin the nightly version using 2. The two new 3. Single test function for all expansion cases pub fn expand() {
macrotest::expand("tests/expanded/avro_*.rs");
}If any snapshot fails, the entire test fails together. Individual 4. No documentation for updating snapshots There is no mention of how to regenerate snapshots when the macro output legitimately changes. Consider adding a comment in Minor notes
SummaryThe core approach is sound and a real improvement in test quality. The main actionable issue before merging is concern 1: without pinning the nightly version, these snapshot tests will generate random CI failures requiring regeneration that is unrelated to actual code changes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces macrotest to test the expanded macro code, replacing the previous inline tests. The existing tests are migrated to this new framework, using snapshot testing for successful expansions and UI tests for compile-time failures. Additionally, the generated code from the derive macro is updated to use fully qualified paths for types from the standard library, which makes the macro expansion more robust.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| ::apache_avro::schema::Schema::Enum(apache_avro::schema::EnumSchema { | ||
| name: ::apache_avro::schema::Name::new(#full_schema_name).expect(&format!("Unable to parse enum name for schema {}", #full_schema_name)[..]), | ||
| ::apache_avro::schema::Schema::Enum(::apache_avro::schema::EnumSchema { | ||
| name, |
There was a problem hiding this comment.
Enum schema name now differs from record schema name behavior
Medium Severity
The EnumSchema name field now uses the name variable from handle_named_schemas, which is created via Name::new_with_enclosing_namespace (inherits enclosing namespace). Previously it used Name::new(full_schema_name) (ignores enclosing namespace), matching the RecordSchema behavior which still creates its own name via Name::new on line 255 of lib.rs. For enums without an explicit namespace nested inside a namespaced type, the enum's schema name will now include the parent's namespace, while records in the same scenario will not.
Additional Locations (1)
There was a problem hiding this comment.
value:annoying; category:bug; feedback: The Bugbot AI reviewer is not correct! The new way of constructing the Name is similar to the old one - it still passes the full_schema_name, so it won't use any enclosing namespace if the provided name has a namespace.
| fields: schema_fields, | ||
| lookup, | ||
| attributes: Default::default(), | ||
| attributes: ::std::collections::BTreeMap::new(), |
There was a problem hiding this comment.
Incomplete path qualification in record_fields quotation
Low Severity
The record_fields quotation uses non-fully-qualified Vec and Some, while the schema_def quotation (line 251) was updated to use ::std::vec::Vec. This inconsistency in the same function appears to be an incomplete refactoring, since the whole point of these changes was to use fully qualified paths for macrotest compatibility.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct! Rust macro quoted code should use fully qualified names to prevent clashes with symbols defined in the user code, this is the so called "macro hygiene". Prevents eventual build issues in applications which provide their own structs with names Vec and Some
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
avro_derive/src/lib.rs (1)
249-275:⚠️ Potential issue | 🟡 MinorFully qualify the remaining
record_fieldsreturn path symbols.Lines 272 and 274 emit unqualified
VecandSome; these can be shadowed in user scope and cause macro expansion failures. Theschema_defblock at line 251 correctly uses::std::vec::Vec, and the same qualification is needed here for consistency.Required fix
let record_fields = quote! { - let mut schema_fields = Vec::with_capacity(`#minimum_fields`); + let mut schema_fields = ::std::vec::Vec::with_capacity(`#minimum_fields`); #(`#record_field_exprs`)* - Some(schema_fields) + ::std::option::Option::Some(schema_fields) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avro_derive/src/lib.rs` around lines 249 - 275, The record_fields quote block uses unqualified Vec and Some which can be shadowed; update the block (symbol: record_fields) to fully qualify those paths to match schema_def—use ::std::vec::Vec::with_capacity(`#minimum_fields`) (or ::std::vec::Vec) instead of Vec and ::std::option::Option::Some(...) instead of Some(...); ensure the `#record_field_exprs` insertion remains unchanged.
🧹 Nitpick comments (2)
avro_derive/tests/ui/avro_rs_501_tuple_struct.rs (1)
20-23: LGTM!Correctly structured UI test verifying that the
AvroSchemaderive macro rejects tuple structs with the appropriate error message.Minor style nit: the space between the struct name and tuple fields (
struct B (i32, String)) is unconventional. Standard Rust style isstruct B(i32, String). Not blocking since this is a test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avro_derive/tests/ui/avro_rs_501_tuple_struct.rs` around lines 20 - 23, Update the tuple struct declaration used in the UI test to follow conventional Rust spacing: remove the space between the struct name and the tuple fields in the #[derive(AvroSchema)] struct B declaration (the symbol to change is the tuple struct "struct B (i32, String)" in the test), rewriting it as "struct B(i32, String)" so the test file uses standard Rust style while preserving the same semantics and error expectation.avro_derive/tests/expand.rs (1)
22-27: LGTM!Well-designed test harness with proper nightly gating via
rustversion::attr. The approach of includingmod expandedto verify the unexpanded code compiles, combined withmacrotest::expandto verify the expansion output, provides comprehensive coverage.The
pubvisibility on the test function is unnecessary since test functions don't need to be public. Consider removing it for consistency:Suggested change
#[rustversion::attr(not(nightly), ignore)] #[test] -pub fn expand() { +fn expand() { macrotest::expand("tests/expanded/avro_*.rs"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avro_derive/tests/expand.rs` around lines 22 - 27, The test function is unnecessarily public; change the signature of the test function named expand from "pub fn expand()" to a private test function "fn expand()" (leave the attributes #[rustversion::attr(not(nightly), ignore)] and #[test] and the macrotest::expand(...) call intact) so the function has default visibility for tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@avro_derive/src/lib.rs`:
- Around line 249-275: The record_fields quote block uses unqualified Vec and
Some which can be shadowed; update the block (symbol: record_fields) to fully
qualify those paths to match schema_def—use
::std::vec::Vec::with_capacity(`#minimum_fields`) (or ::std::vec::Vec) instead of
Vec and ::std::option::Option::Some(...) instead of Some(...); ensure the
`#record_field_exprs` insertion remains unchanged.
---
Nitpick comments:
In `@avro_derive/tests/expand.rs`:
- Around line 22-27: The test function is unnecessarily public; change the
signature of the test function named expand from "pub fn expand()" to a private
test function "fn expand()" (leave the attributes
#[rustversion::attr(not(nightly), ignore)] and #[test] and the
macrotest::expand(...) call intact) so the function has default visibility for
tests.
In `@avro_derive/tests/ui/avro_rs_501_tuple_struct.rs`:
- Around line 20-23: Update the tuple struct declaration used in the UI test to
follow conventional Rust spacing: remove the space between the struct name and
the tuple fields in the #[derive(AvroSchema)] struct B declaration (the symbol
to change is the tuple struct "struct B (i32, String)" in the test), rewriting
it as "struct B(i32, String)" so the test file uses standard Rust style while
preserving the same semantics and error expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 000dd45c-0b2a-406a-b646-9e6525cc44fb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.github/workflows/test-lang-rust-ci.ymlavro_derive/Cargo.tomlavro_derive/src/enums/plain.rsavro_derive/src/lib.rsavro_derive/tests/expand.rsavro_derive/tests/expanded/avro_3687_basic_enum_with_default.expanded.rsavro_derive/tests/expanded/avro_3687_basic_enum_with_default.rsavro_derive/tests/expanded/avro_3709_record_field_attributes.expanded.rsavro_derive/tests/expanded/avro_3709_record_field_attributes.rsavro_derive/tests/expanded/avro_rs_207_rename_all_attribute.expanded.rsavro_derive/tests/expanded/avro_rs_207_rename_all_attribute.rsavro_derive/tests/expanded/avro_rs_207_rename_attr_over_rename_all_attribute.expanded.rsavro_derive/tests/expanded/avro_rs_207_rename_attr_over_rename_all_attribute.rsavro_derive/tests/expanded/avro_rs_501_basic.expanded.rsavro_derive/tests/expanded/avro_rs_501_basic.rsavro_derive/tests/expanded/avro_rs_501_namespace.expanded.rsavro_derive/tests/expanded/avro_rs_501_namespace.rsavro_derive/tests/expanded/avro_rs_501_reference.expanded.rsavro_derive/tests/expanded/avro_rs_501_reference.rsavro_derive/tests/expanded/avro_rs_501_struct_with_optional.expanded.rsavro_derive/tests/expanded/avro_rs_501_struct_with_optional.rsavro_derive/tests/expanded/mod.rsavro_derive/tests/ui/avro_3687_basic_enum_with_default_twice.rsavro_derive/tests/ui/avro_3687_basic_enum_with_default_twice.stderravro_derive/tests/ui/avro_rs_501_non_basic_enum.rsavro_derive/tests/ui/avro_rs_501_non_basic_enum.stderravro_derive/tests/ui/avro_rs_501_tuple_struct.rsavro_derive/tests/ui/avro_rs_501_tuple_struct.stderravro_derive/tests/ui/avro_rs_501_unit_struct.rsavro_derive/tests/ui/avro_rs_501_unit_struct.stderrlicenserc.toml
🤖 Augment PR SummarySummary: This PR switches derive-macro verification to macro-expansion based testing using Changes:
Technical Notes: Expansion and UI tests are marked nightly-only because expanded output can vary with toolchain/tooling versions. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| - uses: taiki-e/install-action@v2 | ||
| if: matrix.rust == 'nightly' | ||
| with: | ||
| tool: cargo-expand |
There was a problem hiding this comment.
macrotest comparisons are sensitive to cargo expand output; installing an unpinned cargo-expand version can make nightly CI fail when cargo-expand changes its formatting even if your code hasn’t. Consider pinning a specific cargo-expand version (as macrotest recommends) to reduce test flakiness.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! Both macrotest and cargo-expand recommend using a pinned version to prevent random build failures when a new version is released. Prevents test failures when unrelated code changes are made.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! There is no need the test function to be public. It does not do any harm either. |
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! There is no need of the space between the struct name and its tuple parameters. Since |
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! Rust macro quoted code should use fully qualified names to prevent clashes with symbols defined in the user code, this is the so called "macro hygiene". Prevents eventual build issues in applications which provide their own structs with names Vec and Some |
2 participants
501: To review by AI