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

[RFC] Adopt cargo nextest #2131

Open
yihuaf opened this issue Jul 5, 2023 · 10 comments
Open

[RFC] Adopt cargo nextest #2131

yihuaf opened this issue Jul 5, 2023 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers RFC

Comments

@yihuaf
Copy link
Collaborator

yihuaf commented Jul 5, 2023

cargo nextest is a improved version of test runners compared to the default cargo test and it is a drop in replacement. The home page summarizes the benefit well: https://nexte.st/index.html#features. There is not clear down side I can think of.

Specifically, I like the speed and the leaky test detection. The speed is noticeably faster. This can reduce the test to under 2 seconds running all unit tests. The leaky test detection is useful because we launch processes and threads in our unit tests. This allows us to detect processes that are not correctly terminated in the test, which almost always indicates a bug in our code. For example, this PR (#1948) was the result of me playing with cargo nextest a while back and noticed that one of the test related to foreground mode is leaky.

@yihuaf yihuaf assigned yihuaf and unassigned yihuaf Jul 5, 2023
@yihuaf yihuaf added enhancement New feature or request good first issue Good for newcomers RFC labels Jul 5, 2023
@utam0k
Copy link
Member

utam0k commented Jul 5, 2023

There is not clear down side I can think of.

It seems that cargo nextest doesn't support doc tests yet.
nextest-rs/nextest#16

@yihuaf
Copy link
Collaborator Author

yihuaf commented Jul 5, 2023

Hmmm... Good catch. We do have the option to add an extra step with cargo test --docs for the doc test only as pointed out by the comments. Alternatively we can revisit this. I will try to run nextest on the side periodically when I suspect there is leaky test.

@yihuaf yihuaf removed the good first issue Good for newcomers label Jul 5, 2023
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jul 5, 2023

Another benefit of using nextest is to impose a timeout on specific tests. cargo test can't terminate with a timeout when test hangs. For us this is beneficial because we do deal with a lot of concurrencies involving processes and channels.

@utam0k
Copy link
Member

utam0k commented Jul 6, 2023

Does it have any impact on our test coverage? I'd also like to shorten the CI time so I'd rather introduce it if we can clear everything.

@yihuaf
Copy link
Collaborator Author

yihuaf commented Jul 8, 2023

coverage

Coverage is supported: https://nexte.st/book/test-coverage.html

I believe only the doc test requires a bit extra workaround which to me should be acceptable. If the general direction is OK, then I will put together a PR for review.

@yihuaf yihuaf added the good first issue Good for newcomers label Jul 8, 2023
@utam0k
Copy link
Member

utam0k commented Jul 8, 2023

Let's go ahead 🚀

@utam0k
Copy link
Member

utam0k commented Jul 8, 2023

@YJDoc2
I'd like to get your opinion. Do you have anything?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 10, 2023

Hey, I had taken look at nextest a while back, and it seems pretty good. It supports most of the cargo test features, and is pretty fast as well. I think we should go ahead with the split approach of using nextest for unitests, and then running doctests via cargo test. We might be able to just split the just-recipe into two steps, so the actual invocation stays the same.

I also have a suggestion, although not sure if it would be possible : Can we have a default fallback in justfile itself? So when we run just unittest it will detect if nextest is installed on system, and if not , simply use cargo test instead. That way users don't have to install nextest. Unless we are depending on some specific features of nextest apart from its ability to run tests in parallel, I think it'd be a good idea to give devs fallback if they don't want to install nextest.

@yihuaf some of our tests need to be run in strictly serial mode, so please check that they are getting correctly run, apart from that I think we should go ahead!

On a separate note, I'm also thinking of contributing to nextest to detect all kinds of leaked process, and if that goes well ; then we'd be able to detect and prevent a lot of leakage via unit tests themselves.

@yihuaf
Copy link
Collaborator Author

yihuaf commented Jul 10, 2023

some of our tests need to be run in strictly serial mode, so please check that they are getting correctly run, apart from that I think we should go ahead!

So far I have not seen any issue.

@Astlaan
Copy link

Astlaan commented Feb 16, 2024

Well, I wouldn't maybe just put nextest as a drop-in replacement.

The reason is that sometimes when testing you have a computationally expensive setup procedure. If you are using threads, you can just run it once and define it, using for example lazy_static. However, if each function is in a different process, you have to rerun the expensive computation for every test function, since they don't share the memory.

I think the idea of nextest is good in that they don't run one test file per binary sequencially, and then maybe one of the test is slower and then delays everything. However, I think maybe it should just merge the files into a single binary, or allow us to choose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers RFC
Projects
None yet
Development

No branches or pull requests

4 participants