Skip to content

refactor(common): Share Common Macro Utilities#2735

Merged
refcell merged 2 commits into
mainfrom
rf/refactor/shared-macro-cleanups
May 18, 2026
Merged

refactor(common): Share Common Macro Utilities#2735
refcell merged 2 commits into
mainfrom
rf/refactor/shared-macro-cleanups

Conversation

@refcell
Copy link
Copy Markdown
Contributor

@refcell refcell commented May 16, 2026

Summary

This refactor moves duplicated trace capture setup into the protocol test utilities and reuses it from derive tests. It generates the repeated rollup fork activation methods from a shared macro while preserving the public method names. It also adds a shared CLI entrypoint macro for binaries that parse Clap args, run the command, and exit on errors.

Co-authored-by: Codex <noreply@openai.com>
@refcell refcell added consensus Area: consensus tooling Area: tooling labels May 16, 2026
@refcell refcell self-assigned this May 16, 2026
@cb-heimdall
Copy link
Copy Markdown
Collaborator

cb-heimdall commented May 16, 2026

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@vercel
Copy link
Copy Markdown

vercel Bot commented May 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
base Ignored Ignored Preview May 16, 2026 10:52pm

Request Review


mod tracing;
pub use tracing::{CollectingLayer, TraceStorage};
pub use base_protocol::test_utils::{CollectingLayer, TraceStorage};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This re-export is now dead code within the derive crate — all call sites were migrated to base_protocol::capture_traces!(), which references the types directly via $crate::test_utils::*. Unless there are downstream consumers of base_derive::test_utils::{CollectingLayer, TraceStorage}, this line can be removed.

}

/// Installs a temporary tracing subscriber that captures events into [`TraceStorage`].
#[macro_export]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#[macro_export] places this macro at the crate root unconditionally, even though the test_utils module is gated behind #[cfg(any(test, feature = "test-utils"))]. If someone invokes base_protocol::capture_traces!() without the test-utils feature enabled, they'll get a confusing compilation error about $crate::test_utils::TraceStorage not existing, rather than the macro itself being absent.

Not a practical problem today (all callers are in test code), but consider adding a doc comment noting the feature requirement, e.g.:

/// **Requires** the `test-utils` feature (or `#[cfg(test)]`).

Co-authored-by: Codex <noreply@openai.com>
.with(layer)
.with(tracing_subscriber::fmt::layer());
let _guard = tracing::subscriber::set_default(subscriber);
let (trace_store, _guard) = crate::capture_traces!();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The original test_continuing_with_empty_batch was the only test that composed the subscriber with an additional tracing_subscriber::fmt::layer():

let subscriber = tracing_subscriber::Registry::default()
    .with(layer)
    .with(tracing_subscriber::fmt::layer());

The capture_traces!() macro doesn't include the fmt layer, so this test silently lost its stdout trace output. This is harmless (it's a debugging aid, not a correctness concern), but worth noting — if the fmt layer was there intentionally for this specific test, it should be restored manually instead of using the macro.

@refcell refcell requested review from 0x00101010 and danyalprout May 17, 2026 11:53
@refcell refcell marked this pull request as ready for review May 17, 2026 11:54
@refcell refcell enabled auto-merge May 17, 2026 11:54
@github-actions
Copy link
Copy Markdown
Contributor

Review Summary

Clean refactor that consolidates three patterns of duplicated code: rollup fork activation methods, trace-capture test setup, and CLI binary entrypoints. The macro implementations are correct and preserve the original semantics.

Findings (minor)

  1. Dead re-export in derive test_utils (crates/consensus/derive/src/test_utils/mod.rs:40): CollectingLayer and TraceStorage are re-exported from base_protocol but no longer referenced by any code in the derive crate — all call sites now use base_protocol::capture_traces!() directly. This re-export can be removed.

  2. capture_traces! macro visibility vs feature gate (crates/consensus/protocol/src/test_utils.rs:140): #[macro_export] unconditionally places the macro at the crate root, but $crate::test_utils::TraceStorage only resolves when the test-utils feature (or #[cfg(test)]) is active. Not a practical issue today since all callers are test code, but a doc comment noting the feature requirement would prevent confusion.

  3. Lost fmt::layer() in test_continuing_with_empty_batch (crates/consensus/protocol/src/batch/span.rs): This was the only test that composed the subscriber with an additional tracing_subscriber::fmt::layer() for stdout trace output. The capture_traces!() macro doesn't include it. Harmless (debug aid only), but worth noting if the original fmt layer was intentional.

No correctness, safety, or concurrency issues found. The rollup_fork_methods! macro correctly preserves the fork implication chain and the run_cli_main! macro correctly handles both sync and async variants.

@refcell refcell added this pull request to the merge queue May 18, 2026
Merged via the queue into main with commit 3ab81f8 May 18, 2026
25 checks passed
@refcell refcell deleted the rf/refactor/shared-macro-cleanups branch May 18, 2026 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consensus Area: consensus tooling Area: tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants