refactor our test separation and how we run them #12458
Conversation
This fundamentally removes the differentiation between "integration" and "unit" tests from the test runner perspective, as well as removes the reliance on the "expensive-tests" compile-time feature. Instead tests are categorized by the underlying reason for which we had the separation in the first place -- test execution speed. These are now annotated with `slow_test_` and `ultraslow_test_` prefixes, and their execution time is enforced by time limits in `nextest.toml`. By using `nextest` filters we can cleanly filter out tests from these three categories to be run in various contexts. For instance, maybe quick checks locally don't need to run the slow tests every time -- `just nextest` by default now takes just 9 seconds (on my machine) to run all the tests. Whereas previously (due to e.g. estimator smoke test) it would take at least a minute. Then a `just nextest-slow` is added in the rarer cases where one might want to run the full suite that runs on CI. CI will run these mildly slower tests for each PR. Then what remains are the ultraslow tests which only run on nayduck and can take quite some time to complete. These can now be trivially run locally as well with `just nextest-all` without necessarily rebuilding the entire project or "unignoring" tests that are marked as ignored for other reasons (like for instance because they're just broken.)
|
cc @wacban -- I pushed out a draft cause I gotta run now, but feedback would be neat if this addresses your problems with the bls tests. |
nagisa
force-pushed
the
refactors-test
branch
from
29c7497
to
40e9d3a
Compare
.config/nextest.toml
Outdated
| [profile.default] | ||
| slow-timeout = { period = "60s", terminate-after = 3, grace-period = "0s" } | ||
| slow-timeout = { period = "5s", terminate-after = 3, grace-period = "1s" } | ||
| default-filter = "not (test(/^(.*::slow_test|slow_test)/) | test(/^(.*::ultraslow_test|ultraslow_test)/))" |
There was a problem hiding this comment.
Why do you need two regexes, one with .*:: and one without it?
There was a problem hiding this comment.
My understanding is that the tests that are at the package root do not have any prefix to their name, but I haven't actually checked this.
You can find it failing even now at https://github.com/near/nearcore/actions/runs/11845706954/job/33011766898?pr=12458 due to me having missed to include one of the tests! That said, one of the things we could do here at this point is to get rid of |
nagisa
force-pushed
the
refactors-test
branch
from
40e9d3a
to
e0593f4
Compare
nagisa
force-pushed
the
refactors-test
branch
from
79497b0
to
fcfcefc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12458 +/- ##
===========================================
+ Coverage 39.74% 69.85% +30.10%
===========================================
Files 842 838 -4
Lines 170756 169385 -1371
Branches 170756 169385 -1371
===========================================
+ Hits 67872 118319 +50447
+ Misses 98635 45813 -52822
- Partials 4249 5253 +1004
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
I've seen this nayduck error, the fix for me was to use with underscore instead of dash in one place in the expensive.txt: |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Umm… so apparently nayduck's tests are limited to 3 minutes?? So much for ultra slow 🤔 But wait, there are others that take 12… how does that work? |
From expensive.txt it seems like you can increase the default timeout like so: |
This fundamentally removes the differentiation between "integration" and "unit" tests from the test runner perspective, as well as removes the reliance on the "expensive-tests" compile-time feature.
Instead tests are categorized by the underlying reason for which we had the separation in the first place -- test execution speed. These are now annotated with
slow_test_andultraslow_test_prefixes, and their execution time is enforced by time limits innextest.toml.By using
nextestfilters we can cleanly filter out tests from these three categories to be run in various contexts. For instance, maybe quick checks locally don't need to run the slow tests every time --just nextestby default now takes just 9 seconds (on my machine) to run all the tests. Whereas previously (due to e.g. estimator smoke test) it would take at least a minute.Then a
just nextest-slowis added in the rarer cases where one might want to run the full suite that runs on CI. CI will run these mildly slower tests for each PR.Then what remains are the ultraslow tests which only run on nayduck and can take quite some time to complete. These can now be trivially run locally as well with
just nextest-allwithout necessarily rebuilding the entire project or "unignoring" tests that are marked as ignored for other reasons (like for instance because they're just broken.)