Make cargo test usable for nearcore developers
#4490
Comments
matklad
added
C-housekeeping
|
This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. |
|
@matklad this is fixed right? |
|
Not at all:
|
|
@matklad The solution for having many prints is to run the code with By setting proper environment flags you eliminate the issue. |
|
Seems reasonable to flip the default? Run with |
I agree, it's a good idea to switch the default for tests. However, then we should update the CI to run with Also changing the default, would change default settings for |
Yes, this is the issue. When I run tests all I want to see is |
|
Turns out libtest is broken: rust-lang/rust#90785 |
diff --git a/test-utils/logger/src/lib.rs b/test-utils/logger/src/lib.rs
index 7372b5308..6be107180 100644
--- a/test-utils/logger/src/lib.rs
+++ b/test-utils/logger/src/lib.rs
@@ -20,7 +20,7 @@ fn setup_subscriber_from_filter(mut env_filter: EnvFilter) {
let _ = tracing_subscriber::fmt::Subscriber::builder()
.with_span_events(tracing_subscriber::fmt::format::FmtSpan::CLOSE)
.with_env_filter(env_filter)
- .with_writer(std::io::stderr)
+ .with_writer(tracing_subscriber::fmt::TestWriter::new())
.try_init();
}
fixes the issue (a bit swamped rn to submit an actual PR) |
Thanks, I tested it. This works fine: #6023 |
Rust’s `libtest` has a… limitation where it doesn’t capture data written directly to `std::io::stdout` or `std::io::stderr` (see rust-lang/rust#90785). Since we are test logger to use the latter, none of the log messages are captured by the test harness. The solution is to use `TestWriter` as the writer instead which cooperates `libtest` better. Issue: near#4490
Indeed, #6025
The issue with that approach is that if you forget to set the environment variable you end up with a passing test that doesn’t test anything. This is another limitation of libtest which does not allow returning an |
Change logger to print output of `cargo test` correctly. #4490 (comment)
Firstly, the proper way to denote a test as expensive is to use `#[cfg]` directive before its definition rather than adding a return inside of the function. The latter will cause the test to show up in list of tests and always pass giving people false sense that running the test without `expensive_tests` feature actually does something. After fixing `sync_state_nodes_multishard` also add it to nightly run. Secondly, the `check_nightly.py` script is rather crude and the order in which `#[cfg]` and `#[test]` attributes are applied do matter. With `#[test]` being first, the script failed to recognise `test_catchup` as an expensive test. Thirdly, the `check_nightly.py` script also doesn’t understand `#[cfg]` matching multiple features. Because of that it does not detect `test_highload` as an expensive test either. Since we’re not using `regression_tests` feature for anything simply remove it. Issue: #4490
Introduce a `#[nightly_test]` attribute to be used to mark tests as expensive (i.e. to be run nightly on NayDuck but omitted from casual `cargo test` invocations or CI runs). It replaces the use of `#[cfg(feature = "expensive_tests")]` directive and under the hood marks tests as ignored rather than skipping their compilation. This means that while previously nightly tests would be built only if `expensive_tests` feature is set now they are always compiled. This further means that changes that break such tests will be caught by CI (previously they would break only once NayDuck decides to run them) and there is no longer need to conditionally compile helper functions which are used by such tests only. Issue: #4490
Firstly, the proper way to denote a test as expensive is to use `#[cfg]` directive before its definition rather than adding a return inside of the function. The latter will cause the test to show up in list of tests and always pass giving people false sense that running the test without `expensive_tests` feature actually does something. After fixing `sync_state_nodes_multishard` also add it to nightly run. Secondly, the `check_nightly.py` script is rather crude and the order in which `#[cfg]` and `#[test]` attributes are applied do matter. With `#[test]` being first, the script failed to recognise `test_catchup` as an expensive test. Thirdly, the `check_nightly.py` script also doesn’t understand `#[cfg]` matching multiple features. Because of that it does not detect `test_highload` as an expensive test either. Since we’re not using `regression_tests` feature for anything simply remove it. Issue: #4490
Rather than using `cfg` to not compile expensive tests, use `cfg_attr`
to conditionally mark such tests as ignored. In other words, instead
of writing:
#[cfg(feature = "expensive_tests")]
#[test]
fn test_clear_old_data_too_many_heights() {
// ...
}
write:
#[test]
#[cfg_attr(not(feature = "expensive_tests"), ignore)]
fn test_clear_old_data_too_many_heights() {
// ...
}
With this change, expensive tests will always be built which means
that i) any code changes breaking them will be caught by CI (rather
than having to wait for a nightly run) and ii) code used by expensive
tests only is no longer unused when `expensive_tests` feature is not
enabled (which means fewer `#[cfg(feature = "expensive_tests")]`
directives sprinkled throughout code).
Since we no longer mark whole modules as compiled only if the feature
is enabled, this change also means that each individual test needs to
be marked individually (rather than being able to mark whole module).
This makes it more obvious which tests are expensive and which aren’t
(since the marking is right at the test definition site) and
simplifies `check_nightly.py` script.
Issue: #4490
Rather than using `cfg` to not compile expensive tests, use `cfg_attr`
to conditionally mark such tests as ignored. In other words, instead
of writing:
#[cfg(feature = "expensive_tests")]
#[test]
fn test_clear_old_data_too_many_heights() {
// ...
}
write:
#[test]
#[cfg_attr(not(feature = "expensive_tests"), ignore)]
fn test_clear_old_data_too_many_heights() {
// ...
}
With this change, expensive tests will always be built which means
that i) any code changes breaking them will be caught by CI (rather
than having to wait for a nightly run) and ii) code used by expensive
tests only is no longer unused when `expensive_tests` feature is not
enabled (which means fewer `#[cfg(feature = "expensive_tests")]`
directives sprinkled throughout code).
Since we no longer mark whole modules as compiled only if the feature
is enabled, this change also means that each individual test needs to
be marked individually (rather than being able to mark whole module).
This makes it more obvious which tests are expensive and which aren’t
(since the marking is right at the test definition site) and
simplifies `check_nightly.py` script.
Issue: #4490
|
#5817 is related -- tests not being flaky is a part of |
Rather than using `cfg` to not compile expensive tests, use `cfg_attr`
to conditionally mark such tests as ignored. In other words, instead
of writing:
#[cfg(feature = "expensive_tests")]
#[test]
fn test_clear_old_data_too_many_heights() {
// ...
}
write:
#[test]
#[cfg_attr(not(feature = "expensive_tests"), ignore)]
fn test_clear_old_data_too_many_heights() {
// ...
}
With this change, expensive tests will always be built which means
that i) any code changes breaking them will be caught by CI (rather
than having to wait for a nightly run) and ii) code used by expensive
tests only is no longer unused when `expensive_tests` feature is not
enabled (which means fewer `#[cfg(feature = "expensive_tests")]`
directives sprinkled throughout code).
Since we no longer mark whole modules as compiled only if the feature
is enabled, this change also means that each individual test needs to
be marked individually (rather than being able to mark whole module).
This makes it more obvious which tests are expensive and which aren’t
(since the marking is right at the test definition site) and
simplifies `check_nightly.py` script.
Issue: #4490
|
For the future, by running |
|
@matklad Thanks, that's really useful. I'll add it to my scripts. |
I've noticed that I never run
cargo test --workspacelocally. I do want to have an ability to quickly check if I broke something somewhere, but the usability of our currentcargo testis abysmal.Specifically:
I suggest immediate AIs:
cargo testis never polluted unless the test actually fails. I think usingTestWriteris one possible way to do thisexpensive_testsLonger term, I think it might sense to do the following:
expensive_testswith run-time check (see the example at the end of "Make Tests Fast" section of this post).expensive_tests--nightly_only_tests(> 5 min) andci_only_tests(>10s, <5min). Localcargo tskipsci_only_testsby default, but they are run by CI for every commit that gets into mastercargo ttakes 5s wall-clock/35s cpu by default and 30s/100s when including ci only tests.The text was updated successfully, but these errors were encountered: