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

Add custom env struct to store env at init #1025

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

wescande
Copy link
Contributor

This allow to no longer ignore some tests (marked previously as FIXME)
by either storing the env at the start of the program or creating a custom env
for test purpose.
This centralize almost all calls to std::env inside one wrapper


Add a test profile to increase speed for testing (5min -> 20sec on my
machine)


clean a few nit in code style like this:

if Some(value) = ...
    if value

to

if Some(true) = ...

This allow to no longer ignore some tests (marked previously as FIXME)
by storing the env at the start of the program (Or creating a custom env
for test purpose)
This centralize almost alls calls to std::env inside one wrapper

Add a test profile to increase speed for testing (5min -> 20sec on my
machine)

clean a few code style like this:
```
if Some(value) = ...
    if value
```
to
```
if Some(true) = ...
```
@wescande
Copy link
Contributor Author

You may consider this as a big edit without a lot of added value. And you will not be wrong! 😂
I don't know why I was digging into this... But here we are.

@dandavison
Copy link
Owner

You may consider this as a big edit without a lot of added value. And you will not be wrong! 😂 I don't know why I was digging into this... But here we are.

😂 haha!

Wow, the test speed up is incredible, e.g. the slowest test test_color_only_output_is_in_one_to_one_correspondence_with_input

running 1 test
test tests::test_example_diffs::tests::test_color_only_output_is_in_one_to_one_correspondence_with_input ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 344 filtered out; finished in 50.55s
running 1 test
test tests::test_example_diffs::tests::test_color_only_output_is_in_one_to_one_correspondence_with_input ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 344 filtered out; finished in 3.52s

And the test suite just finished while I was writing this comment! Normally there are 2 or 3 linux tests on a different architecture that straggle on for ages...

image

Are there any reasons we shouldn't be testing with that optimization level? I hope not :) I can't think of any. I guess Rust defaults to the unoptimized to give people faster edit-compile-test cycles when the compilation is a larger proportion of the time.

@wescande
Copy link
Contributor Author

Are there any reasons we shouldn't be testing with that optimization level?

The book doesn't say much more than "this settings exist".
As for the embedded book, they only talking about speed and LLVM loop optimization.
I found this issue that is good to know.
This answer is also good to know, but we are already doing what it suggest.

I don't see any reason !

@wescande
Copy link
Contributor Author

Also the test you mention is slow mainly because of assets.get_syntax_set() take more than 1 second without the opt.level. (It still the slow point with the opt level, but around 60ms only)
If you multiply that by the number of entry for this test, you get a very slow test compare to other.

But I don't know how to replace this call. 😅

@dandavison
Copy link
Owner

And in addition to the test speed-up, I think your refactoring is absolutely right i.e. that the usage of env vars should be managed in that fashion. Thanks for this work.

@dandavison dandavison merged commit 85a07cd into dandavison:master Mar 31, 2022
@dandavison
Copy link
Owner

Also the test you mention is slow mainly because of assets.get_syntax_set() take more than 1 second without the opt.level. (It still the slow point with the opt level, but around 60ms only)

Interesting. If we did want to speed that up even more then it should be possible to cache it across tests I think.

dandavison added a commit that referenced this pull request Apr 3, 2022
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.

2 participants