Fix lints in integration testing API #340
Merged
Conversation
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
This is to appease clippy.
The `--all-targets` flag is added to the `cargo check` command to make it run checks on all targets, including tests. It must have been accidentally omitted.
Some code related to integration testing is conditionally compiled based on the `cpp_integration_testing` flag, which is not set by default. This made some lints not run on that code, which is undesirable. This commit adds the `cpp_integration_testing` flag to RUSTFLAGS when running clippy, so that all lints are run on that code as well. Keep in mind that `cargo check` is still run without the `cpp_integration_testing` flag. This is done on purpose, to have one basic check for the code with the `cpp_integration_testing` flag off (to make sure it passes them correctly), and one extended (clippy) check for the code with the `cpp_integration_testing` flag on.
There was a problem hiding this comment.
Pull Request Overview
The PR ensures integration testing code is linted and exposed correctly while improving CI checks.
- Appease recent Clippy lints and enable
--all-targetsforcargo check - Expose
integration_testingAPIs undercpp_integration_testing - Standardize formatting calls and update build script and Makefile flags
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/prepared.rs | Simplified panic format using named parameters |
| src/logging.rs | Switched to short-format in write! calls |
| src/integration_testing.rs | Replaced write! + newline with writeln!, made type pub |
| src/api.rs | Added conditional re-exports under integration_testing |
| build.rs | Removed placeholder in println!, now interpolates variable |
| Makefile | Introduced FULL_RUSTFLAGS, enabled --all-targets & integration flags |
Lorak-mmk
approved these changes
Jul 7, 2025
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.
Problem description
When I introduced the
apimodule and turned on optional lints about the (non-)pubtypes, I forgot to exposepubfunctions from theintegration_testingmodule. This was not spotted by the CI, because the checks were run without--cfg cpp_integration_testinginRUSTFLAGS.What's done
integration_testingmodule is made pass the optional lints.cargo checkis now run with the--all-targetsflag, so that it checks tests, too (not only the lib crate).cargo clippyis now run with thecpp_integration_testingoption inRUSTFLAGSto make the integration testing code covered by the linter.Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced.[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.