Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 2 additions & 36 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ borsh = { version = "1.5.1", features = ["derive", "unstable__schema"] }
borsh-derive = "1.5.1"
bs58 = "0.5.1"
byteorder = "1.5.0"
cargo-expand = "1"
cargo-lock = "10.0.0"
chrono = "0.4"
clap = "4.5.40"
Expand Down Expand Up @@ -93,7 +92,6 @@ lazy_static = "1.4.0"
libc = "0.2.153"
log = { version = "0.4.20", features = ["release_max_level_info"] }
lru = "0.16.0"
macrotest = "1"
magic-domain-program = { git = "https://github.com/magicblock-labs/magic-domain-program.git", rev = "ea04d46", default-features = false }
magicblock-account-cloner = { path = "./magicblock-account-cloner" }
magicblock-accounts = { path = "./magicblock-accounts" }
Expand Down
2 changes: 1 addition & 1 deletion magicblock-config-macro/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ serde = { workspace = true, features = ["derive"] }
[dev-dependencies]
magicblock-config-helpers = { workspace = true }
trybuild = { workspace = true }
macrotest = { workspace = true }
syn = { workspace = true, features = ["full"] }
60 changes: 57 additions & 3 deletions magicblock-config-macro/tests/test_merger.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use macrotest::expand;
use magicblock_config_helpers::Merge;
use magicblock_config_macro::Mergeable;
use std::fs;
use syn::{parse_file, File, Item};

// Test struct with fields that have merge methods
#[derive(Debug, Clone, PartialEq, Eq, Default, Mergeable)]
Expand Down Expand Up @@ -70,13 +71,66 @@ fn test_merge_macro_with_non_default_values() {
assert_eq!(config.nested.value, 50);
}

/// Verifies that the Merge trait is properly implemented for the test struct
#[test]
fn test_merge_macro_codegen() {
fn test_merge_macro_generates_valid_impl() {
let t = trybuild::TestCases::new();
t.pass("tests/fixtures/pass_merge.rs");
t.compile_fail("tests/fixtures/fail_merge_enum.rs");
t.compile_fail("tests/fixtures/fail_merge_union.rs");
t.compile_fail("tests/fixtures/fail_merge_unnamed.rs");

expand("tests/fixtures/pass_merge.rs");
// Verify that TestConfig has a Merge implementation
// by checking if the type implements the trait
fn assert_merge<T: Merge>() {}
assert_merge::<TestConfig>();
}

/// Verifies the macro generates Merge impl for structs with named fields
#[test]
fn test_merge_macro_codegen_verification() {
// Load and parse the expanded fixture to verify structure
let source = fs::read_to_string("tests/fixtures/pass_merge.rs")
.expect("Failed to read pass_merge.rs fixture");

let file: File =
parse_file(&source).expect("Failed to parse pass_merge.rs fixture");

// 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"))
} else {
false
}
});

assert!(
has_mergeable_derive,
"Expected struct with #[derive(Mergeable)]"
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


// Verify the test struct is actually defined
let has_test_config = file.items.iter().any(|item| {
if let Item::Struct(item_struct) = item {
item_struct.ident == "TestConfig"
} else {
false
}
});

assert!(
has_test_config,
"Expected TestConfig struct to be defined"
);

// Compile-test the fixtures to ensure error cases work correctly
let t = trybuild::TestCases::new();
t.pass("tests/fixtures/pass_merge.rs");
t.compile_fail("tests/fixtures/fail_merge_enum.rs");
t.compile_fail("tests/fixtures/fail_merge_union.rs");
t.compile_fail("tests/fixtures/fail_merge_unnamed.rs");
}