Repository navigation
refactor: Rust best practices — doctest, test consolidation, Display, must_use, linker docs - #910
Merged
Merged
Conversation
Move non-global_allocator integration tests into tests/it/ modules so each crate compiles one test binary instead of one per file. This reduces test compile time by eliminating redundant linking. Affected crates: - logfwd-core: 3 test files → tests/it/main.rs - logfwd-io: 2 test files → tests/it/main.rs (allocation_churn.rs kept separate) - logfwd-transform: 3 test files → tests/it/main.rs - logfwd: 3 test files → tests/it/main.rs (allocation_e2e/scaling kept separate) - logfwd-output: 2 test files → tests/it/main.rs (allocation_regression kept separate) Files using #[global_allocator] remain as standalone binaries since only one global allocator per binary is allowed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… must_use, linker docs Apply Rust community best practices from API Guidelines, matklad's build optimization advice, and blessed.rs recommendations: Build & compile time: - Add [lib] doctest = false to all 9 internal crates (each doctest compiles as a separate binary — unnecessary for unpublished crates with unit tests) - Consolidate integration tests into single binary per crate (tests/it/main.rs) for logfwd-core (3 files), logfwd-io (2), logfwd-transform (3), logfwd (3), logfwd-output (2). Tests with #[global_allocator] kept separate. Reduces test link count from 18 binaries to 10. (Ref: matklad) - Document faster linker setup (mold/lld) in DEVELOPING.md as opt-in tip API quality: - Add Display impls for InputType, OutputType, Format config enums (Rust API Guidelines C-COMMON-TRAITS) - Add #[must_use] to ConfigError and SendResult types (Rust API Guidelines C-MUST-USE — types only, not Result-returning fns which already inherit must_use from Result itself) Closes #892 Closes #893 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR implements three compile-time optimization strategies across the workspace: disabling doc tests via Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (4 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-config/src/lib.rs`:
- Around line 122-137: The Display impl for OutputType duplicates the strings
returned by the helper output_type_name; update the Display implementation to
delegate to that helper instead of repeating the match. Locate impl fmt::Display
for OutputType and replace its match body so it calls
f.write_str(output_type_name(self)), keeping output_type_name as the single
source of truth for the &str names (or conversely make output_type_name call
self.to_string() if you prefer Display-driven behavior).
In `@crates/logfwd-output/tests/it/main.rs`:
- Around line 1-2: Gate the ES-dependent module so integration runs only when
explicitly enabled: replace the unconditional `mod elasticsearch_integration;`
with a feature-gated pair—add `#[cfg(feature =
"elasticsearch_integration_tests")] mod elasticsearch_integration;` and a noop
stub `#[cfg(not(feature = "elasticsearch_integration_tests"))] mod
elasticsearch_integration {}` to keep the module path valid when the feature is
off; also mark or gate tests inside `elasticsearch_integration.rs` (or add
`#[cfg(feature = "elasticsearch_integration_tests")]` at the top of that file)
so CI only runs them when the feature is enabled.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7348363-e326-4e1b-aafa-a4fc95dd66d5
📒 Files selected for processing (31)
DEVELOPING.mdcrates/logfwd-arrow/Cargo.tomlcrates/logfwd-config/Cargo.tomlcrates/logfwd-config/src/lib.rscrates/logfwd-core/Cargo.tomlcrates/logfwd-core/tests/it/compliance_data.rscrates/logfwd-core/tests/it/main.rscrates/logfwd-core/tests/it/scanner_conformance.proptest-regressionscrates/logfwd-core/tests/it/scanner_conformance.rscrates/logfwd-core/tests/it/vectorization_canary.rscrates/logfwd-ebpf-proto/Cargo.tomlcrates/logfwd-io/Cargo.tomlcrates/logfwd-io/tests/it/checkpoint_state_machine.rscrates/logfwd-io/tests/it/main.rscrates/logfwd-io/tests/it/transport_e2e.rscrates/logfwd-output/Cargo.tomlcrates/logfwd-output/src/sink.rscrates/logfwd-output/tests/it/elasticsearch_arrow_ipc.rscrates/logfwd-output/tests/it/elasticsearch_integration.rscrates/logfwd-output/tests/it/main.rscrates/logfwd-test-utils/Cargo.tomlcrates/logfwd-transform/Cargo.tomlcrates/logfwd-transform/tests/it/json_udf_tests.rscrates/logfwd-transform/tests/it/main.rscrates/logfwd-transform/tests/it/raw_first_bench.rscrates/logfwd-transform/tests/it/scanner_datafusion_boundary.rscrates/logfwd/Cargo.tomlcrates/logfwd/tests/it/compliance.rscrates/logfwd/tests/it/compliance_file.rscrates/logfwd/tests/it/integration.rscrates/logfwd/tests/it/main.rs
Comment on lines
+1
to
+2
| mod elasticsearch_arrow_ipc; | ||
| mod elasticsearch_integration; |
There was a problem hiding this comment.
Gate Elasticsearch-dependent tests before including elasticsearch_integration.
Line 2 includes elasticsearch_integration in default integration execution, but that module contains unignored tests (e.g., crates/logfwd-output/tests/it/elasticsearch_integration.rs, Line 56). This can make CI/environment-default runs flaky or fail when ES is unavailable.
Suggested fix (apply in crates/logfwd-output/tests/it/elasticsearch_integration.rs)
#[test]
+#[ignore = "requires running Elasticsearch (see examples/elasticsearch/)"]
fn elasticsearch_sink_sends_bulk_data() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-output/tests/it/main.rs` around lines 1 - 2, Gate the
ES-dependent module so integration runs only when explicitly enabled: replace
the unconditional `mod elasticsearch_integration;` with a feature-gated pair—add
`#[cfg(feature = "elasticsearch_integration_tests")] mod
elasticsearch_integration;` and a noop stub `#[cfg(not(feature =
"elasticsearch_integration_tests"))] mod elasticsearch_integration {}` to keep
the module path valid when the feature is off; also mark or gate tests inside
`elasticsearch_integration.rs` (or add `#[cfg(feature =
"elasticsearch_integration_tests")]` at the top of that file) so CI only runs
them when the feature is enabled.
… source of truth Address CodeRabbit review: the output_type_name helper duplicated the same strings as the new Display impl for OutputType. Replace all 4 call delete the now-unused helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add elasticsearch_integration_tests feature flag to logfwd-output. The elasticsearch_integration module now only compiles when explicitly enabled with --features elasticsearch_integration_tests, preventing CI from running ES-dependent tests without infrastructure. Addresses CodeRabbit review item 2 on PR #910. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Apr 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies Rust community best practices from the API Guidelines, matklad's build optimization advice, and blessed.rs.
Build & compile time (#892)
[lib] doctest = falseon all 9 internal crates — each doctest compiles as a separate binary, unnecessary for unpublished crates with comprehensive unit tests and Kani proofsConsolidate integration tests into single
tests/it/main.rsper crate:#[global_allocator])Reduces test binary count from 18 → 10 (each re-links the crate, so fewer = faster).
Faster linker documentation in DEVELOPING.md — mold (Linux) and lld (macOS) as opt-in tips (not in
.cargo/config.tomlto avoid breaking builds for devs without them installed)API quality (#893)
Displayimpls forInputType,OutputType,Formatconfig enums (Rust API Guidelines C-COMMON-TRAITS)#[must_use]onConfigErrorandSendResulttypes (C-MUST-USE). Not added to Result-returning functions sinceResultitself is already#[must_use](avoidsdouble_must_useclippy lint).Not changed (intentionally)
missing_docslint — too noisy to enable workspace-wide under-D warnings; should be done incrementally per crate.cargo/config.toml— would break builds for devs without mold/lldmust_use_candidate/return_self_not_must_useclippy lints — deliberatelyallowed in workspace configCI status
cargo fmt --check✅cargo clippy -- -D warnings✅Closes #892
Closes #893