Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable lints for tests only running optimized #6664

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from

Conversation

dknopik
Copy link
Contributor

@dknopik dknopik commented Dec 6, 2024

Issue Addressed

Previously, tests gated with #[cfg(not(debug_assertions))] were not linted, as Clippy does not run in --release mode to save time.

Proposed Changes

This PR enables linting these tests by setting the compiler option -C debug-assertions=no when invoking Clippy. This retains the time saved by not optimizing while linting. Also, this PR fixes allllll the lint errors uncovered by this.

Additional Info

For easier review, this PR is split into several commits:

  1. fix automatically fixable or obvious lints: Most of the new errors were "machine fixable" (stuff like superfluous & or .into()s). Some errors were not machine fixable but really obvious to fix anyway (at my discretion). Feel free to scroll through this anyway. I checked machine fixable occurrences manually to make sure they're sane.
  2. fix suspicious_open_options by removing manual options: Here I modified the code beyond the scope of the lint to make it a bit nicer, but that is a matter of taste.
  3. fix await_holding_locks: Here we held locks across await points, so I had to shuffle things around a bit. In one case, it was annoying to avoid as NaiveAggregationPool is not Cloneable, so I disabled the lint here (as there is some precedent for that in the codebase). (edit: I now realize one lock is used for inter-test ratelimiting, so I reverted and disabled the lint there in a later commit)
  4. avoid failing lint due to now disabled #[cfg(debug_assertions)]: This one is interesting: The newly set compiler flag caused an issue in this non-test code as the None branch was now empty. I pulled the check out of the match and then applied the lint suggestion. (edit: By accident, I flipped the condition which I fixed in a later commit)
  5. reduce future sizes in tests: Some async test functions now had a too large stack size. I mostly sprinkled some Box::pin around some of the invoked futures, and split up the test in one case.

@dapplion
Copy link
Collaborator

This is a great addition!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants