-
Notifications
You must be signed in to change notification settings - Fork 810
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
Conversation
This is a great addition! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
Regarding await_holding_lock
I think it would be good to fix all of these occurrences eventually, as they could lead to flaky tests/random deadlocks. I think the easiest way to do it is to swap the lock out for a tokio
one, which makes the lock acquisition itself async
.
The only reason we have this lint allow
'd in the VC is that it never seems to have caused problems in practice, and it would be a decently complicated refactor to make that lock async.
Merging this now before it gets into conflict with too many other PRs |
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 02cb2d6 |
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:
&
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.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.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 asNaiveAggregationPool
is notClone
able, 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)#[cfg(debug_assertions)]
: This one is interesting: The newly set compiler flag caused an issue in this non-test code as theNone
branch was now empty. I pulled the check out of thematch
and then applied the lint suggestion. (edit: By accident, I flipped the condition which I fixed in a later commit)Box::pin
around some of the invoked futures, and split up the test in one case.