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

RUST-580 Improve compile times of the test suite #412

Merged

Conversation

patrickfreed
Copy link
Contributor

RUST-580

This PR reimplements the various test runner TestOperation traits manually rather than relying on async_trait, resulting in much faster compile times of the tests. For context, see rust-lang/rust#87012 (comment).

On my machine, an incremental change to TestClient::new (e.g. adding a single println!) used to take ~1m15s to run cargo check --tests, whereas the same change now takes ~10s. There's still a lot of room for improvement (such a change should ideally be near instant), but this is still a big difference.

@patrickfreed patrickfreed marked this pull request as ready for review July 30, 2021 18:28
fn execute_test_runner_operation<'a>(
&'a self,
_test_runner: &'a mut TestRunner,
) -> BoxFuture<'a, ()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this (and likewise below) need a default body? It didn't have one before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They didn't have them previously because default implementations didn't place nicely with async_trait, see this comment for more info: https://github.com/mongodb/mongo-rust-driver/blob/master/src/test/spec/v2_runner/operation.rs#L43.

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! this is awesome, these changes speed up compile time to ~10 seconds for me as well

Copy link
Contributor

@NBSquare NBSquare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Based on the issue you linked it does seem like it's still worth investigating why async_trait causes this issue. Avoiding use of the trait definitely hurts the readability of the test code IMO.

@patrickfreed patrickfreed merged commit 295f3be into mongodb:master Jul 30, 2021
@patrickfreed
Copy link
Contributor Author

Yeah definitely, I updated the original issue and also filed one on async_trait: dtolnay/async-trait#174

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

Successfully merging this pull request may close these issues.

4 participants