chore: clear remaining clippy lints and stop compiling tests in non-test builds - #651
Merged
Conversation
…est builds
`cargo clippy --all-targets -- -D warnings` was failing on `main` due to a
handful of pre-existing lints in test code. CI's `cargo clippy -- -D warnings`
hid these because it does not lint test targets, but anyone running clippy
locally with `--all-targets` (or doing a `cargo test`-time clippy) hit them.
Changes:
- `clippy::assertions_on_constants` (6 occurrences) — replace placeholder
`assert!(true)` bodies in `cli/handlers/update/{tests,updater}.rs`. Three
mock-shaped tests that genuinely had nothing to assert are now empty bodies
marked `#[ignore = "placeholder: ..."]` so they show up under
`cargo test -- --ignored` but no longer pollute the default run. Trivial
"did construction succeed" tests drop the redundant `assert!(true)` since
reaching the line already proves the test passed.
- `clippy::module_inception` —
`cloud_providers/aws/pricing/tests.rs` previously wrapped its contents in an
inner `mod tests { ... }`, producing the path
`pricing::tests::tests`. Removed the wrapper; the file is itself the
`tests` module.
- `cloud_providers/aws/pricing/mod.rs` declared the `tests` submodule with
`pub mod tests;`, which compiled the test code in non-test builds and
re-exported it. Switched to `#[cfg(test)] mod tests;` so test code is
test-only and private.
Verification (post-PR #650 main):
- `cargo clippy --all-targets -- -D warnings` -> clean (was 6 warnings → -D
warnings made them errors).
- `cargo clippy -- -D warnings` (CI's exact command) -> clean.
- `cargo fmt --all -- --check` -> clean.
- `cargo test --features test-bins` -> all suites pass; 106 lib tests pass
with 5 ignored (3 newly-marked placeholders + 2 pre-existing).
- `cargo audit` -> 0 vulnerabilities.
Co-authored-by: Cursor <cursoragent@cursor.com>
Deploying tracer-client with
|
| Latest commit: |
49a344a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a5e7244d.tracer-client.pages.dev |
| Branch Preview URL: | https://fix-clippy-cleanup.tracer-client.pages.dev |
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
1 participant
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.
Follow-up to #650.
mainis now clean oncargo clippy -- -D warnings(CI's command), but clippy with--all-targetswas still emitting 7 warnings in test code that's pre-existing. CI's check hides them because it doesn't lint test targets — anyone running clippy locally with--all-targets(or runningcargo test-time clippy) hits them.This PR clears them all and fixes one related correctness issue.
Changes
clippy::assertions_on_constants(6x) — replace placeholderassert!(true)bodies incli/handlers/update/{tests,updater}.rs:test_update_impl_error_handling,test_update_impl_success_path,test_sentry_error_reporting) had nothing to assert; the comments explicitly described them as "placeholders for the testing structure". Now empty bodies marked#[ignore = "placeholder: ..."]so they show up undercargo test -- --ignoredwithout polluting the default run.assert!(true)since reaching the line already proves the test passed.test_update_impl_structurebecomestest_update_impl_signature_is_stableand asserts the function signature compiles tofn() -> Result<()>, which is what the original comment said the test was meant to do.clippy::module_inception—cloud_providers/aws/pricing/tests.rspreviously wrapped its contents in an innermod tests { ... }, producing the pathpricing::tests::tests. Removed the wrapper; the file is itself thetestsmodule.cloud_providers/aws/pricing/mod.rsdeclared thetestssubmodule withpub mod tests;, which compiled the test code in non-test builds and re-exported it publicly. Switched to#[cfg(test)] mod tests;so test code is test-only and private. This is a small but real bug — the test module'smock_metadata,setup_client, etc. were being included in release binaries.Verification
cargo clippy --all-targets -- -D warnings→ clean (was 7 warnings → 7 errors with-D)cargo clippy -- -D warnings(CI's exact command) → cleancargo fmt --all -- --check→ cleancargo test --features test-bins→ all suites pass; 106 lib tests pass, 5 ignored (3 newly-marked placeholders + 2 pre-existing#[ignore]d pricing tests)cargo audit→ 0 vulnerabilitiesTest plan
Linter Check(Build and Lint) greenTests(Build and Test) greenRust Cargo Auditgreen (already daily-passing post-security: fix all open Dependabot alerts and pin third-party Actions #650)Made with Cursor