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

Merged
merged 9 commits into from
Dec 17, 2024

Conversation

dknopik
Copy link
Member

@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!

Copy link
Member

@michaelsproul michaelsproul left a 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.

@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Dec 16, 2024
@michaelsproul
Copy link
Member

Merging this now before it gets into conflict with too many other PRs

@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Dec 16, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 02cb2d6

mergify bot added a commit that referenced this pull request Dec 16, 2024
@mergify mergify bot merged commit 02cb2d6 into sigp:unstable Dec 17, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants