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

use nextest for better test isolation #8207

Merged
merged 5 commits into from
Dec 16, 2022
Merged

Conversation

pompon0
Copy link
Contributor

@pompon0 pompon0 commented Dec 12, 2022

it will allow us to set panic=abort per test (because each test is executed in a separate subprocess) and set test timeouts without developing a custom test harness. Additionally I've set the default test timeout to 2min (period = 1min, killing test after 2 periods), which seems reasonable, given that all presubmit tests execute <1min as of today.

@pugachAG
Copy link
Contributor

+1 for enabling this to set timeout for individual unit tests.

I was debugging flaky peer_manager::tests::routing::archival_node test and started preparing a PR to set timeout for cargo test using RUST_TEST_TIME_UNIT env variable along with -Z unstable-options --ensure-time, but this seems like a nicer solution!

@pompon0 pompon0 requested a review from nagisa December 16, 2022 11:02
@pompon0 pompon0 marked this pull request as ready for review December 16, 2022 11:02
@pompon0 pompon0 requested a review from a team as a code owner December 16, 2022 11:02
@pompon0 pompon0 requested a review from mm-near December 16, 2022 11:08
@nagisa
Copy link
Collaborator

nagisa commented Dec 16, 2022

I would argue that we want to adjust documentation to inform developers to use nextest locally as a followup as well. Using different test runners can and will lead to weird hard to diagnose issues (why is it passing on CI but not locally?) eventually.

@akhi3030
Copy link
Collaborator

I would argue that we want to adjust documentation to inform developers to use nextest locally as a followup as well. Using different test runners can and will lead to weird hard to diagnose issues (why is it passing on CI but not locally?) eventually.

+1. Please add some documentation in https://near.github.io/nearcore/practices/testing/index.html (https://github.com/near/nearcore/tree/master/docs/practices/testing)

@pompon0
Copy link
Contributor Author

pompon0 commented Dec 16, 2022

(why is it passing on CI but not locally?)

We have that already, even though we are using the same test harness in CI and locally. Updated documentation.

@near-bulldozer near-bulldozer bot merged commit 291f1fc into master Dec 16, 2022
@near-bulldozer near-bulldozer bot deleted the gprusak-nextest branch December 16, 2022 12:09
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Dec 19, 2022
it will allow us to set panic=abort per test (because each test is executed in a separate subprocess) and set test timeouts without developing a custom test harness. Additionally I've set the default test timeout to 2min (period = 1min, killing test after 2 periods), which seems reasonable, given that all presubmit tests execute <1min as of today.
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.

5 participants