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 cfg_attr to ignore expensive tests #6032

Merged
merged 10 commits into from
Jan 13, 2022
Merged

Use cfg_attr to ignore expensive tests #6032

merged 10 commits into from
Jan 13, 2022

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Jan 9, 2022

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

@mina86 mina86 requested a review from matklad January 9, 2022 07:00
@mina86 mina86 added A-CI Area: Continuous Integration A-testing Area: Unit testing / integration testing T-node Team: issues relevant to the node experience team labels Jan 9, 2022
@matklad
Copy link
Contributor

matklad commented Jan 10, 2022

Definitely an improvement!

I have doubts about introducing a proc macro for this, as this seems a too big of a solution for the problem at hand. I personally would strongly prefer open-coding #[cfg_attr(not(feature = "expensive_tests"), ignore)] on every call-site. My arguments for these, none of which is super strong:

  • For a contributor, seeing #[cfg_attr(not(feature = "expensive_tests"), ignore)] immediately shows how to actually run the thing, and what's going on in general. In contracts, #[nightly_test] is a rather opaque thing.
  • IDEs hate proc-macros. Interestengly, just today rust-analyzer gained an ability to deal with such macros nicely (by ignoring them) feat: Add config to replace specific proc-macros with dummy expanders rust-lang/rust-analyzer#11193, https://rust-analyzer.github.io/thisweek/2022/01/10/changelog-111.html. This still requires manual configuration though.
  • When cargo added similar proc-macro decorator for tests, that measurably added to the compile time. Though, in the case of cargo they added it to every test, and not to a small subset, like we are doing here.
  • Slippery slope -- proc macros generally tend to make things harder to work with, and adding more simple proc-macros creates incentive for adding more proc macros

Admittedly, none of the arguments is particularly strong, so approving.

@matklad
Copy link
Contributor

matklad commented Jan 10, 2022

One more weak contra argument -- this has to be a non-dev dependency, which feels not quite right.

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
@mina86
Copy link
Contributor Author

mina86 commented Jan 10, 2022

@matklad, PTAL

Copy link
Contributor

@pmnoxx pmnoxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I agree with @matklad this looks much better with proc macro. I found out that you can't run tests directly inside CLion, due to macros not being handled properly.

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mina86 mina86 changed the title Introduce a #[nightly_test] attribute Use cfg_attr to ignore expensive tests Jan 11, 2022
@mina86 mina86 changed the title Use cfg_attr to ignore expensive tests Use cfg_attr to ignore expensive tests Jan 11, 2022
scripts/check_nightly.py Show resolved Hide resolved
@near-bulldozer near-bulldozer bot merged commit ec63b2d into master Jan 13, 2022
@near-bulldozer near-bulldozer bot deleted the mpn-nightly branch January 13, 2022 23:35
@gmilescu gmilescu added the Node Node team label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: Continuous Integration A-testing Area: Unit testing / integration testing Node Node team T-node Team: issues relevant to the node experience team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants