Avoid iterating files when `--exact` is passed in #49

zaneduffield · 2024-04-23T12:24:24Z

This fixes a quadratic performance issue in which the test directories are iterated for every test case when run under nextest.

Fixes #8.

zaneduffield · 2024-04-23T12:27:34Z

@sunshowers I wasn't exactly sure how the testing should be implemented.

The only existing test justs run the harness! macro over a test directory and doesn't really assert anything. I figure that there would need to be some higher level integration test that runs another binary built with the harness! macro (assuming that the system has cargo-nextest installed), and makes assertions about its output.

Is that the sort of thing you had in mind?

sunshowers

This is awesome! Thank you, just a couple of minor comments, and some notes below.

The only existing test justs run the harness! macro over a test directory and doesn't really assert anything. I figure that there would need to be some higher level integration test that runs another binary built with the harness! macro (assuming that the system has cargo-nextest installed), and makes assertions about its output.

Haha, good question. Definitely something we'd like to address eventually with the libtest JSON output or similar, but we're not quite there yet. In particular we don't currently have deserializers for the libtest format; nextest-rs/nextest#1152 has tests for it.

But we can definitely do some kinds of testing. Some things to test against would be:

in the test environment, we have an assertion against hitting the full scan (this can be controlled via an internal-only env var)
the human-readable output has the right "Summary" line in it. I know it's documented to not be stable -- but it is part of the nextest org, and if it changes we can just fix it.

sunshowers · 2024-04-23T18:25:40Z

src/runner.rs

+            .collect();
+
+        match exact_tests.len() {
+            0 if is_nextest() => {


Could this just have an outer if is_nextest() condition?

We could, and that would be fine.
I wrote it like this because it does the cheap part (length comparison) first and the (relatively) expensive part (env var string comparison) second. It's never going to matter performance-wise to just always check the environment variable. I'll do it that way.

sunshowers · 2024-04-23T18:26:07Z

src/runner.rs

+}
+
+fn is_nextest() -> bool {
+    std::env::var_os("NEXTEST").is_some_and(|val| val.eq("1"))


Can this be val == "1"?

codecov · 2024-04-23T18:47:57Z

Codecov Report

Attention: Patch coverage is 93.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 88.69%. Comparing base (2c6057a) to head (32942ad).
Report is 6 commits behind head on main.

Files	Patch %	Lines
src/runner.rs	86.27%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   86.33%   88.69%   +2.36%     
==========================================
  Files           4        4              
  Lines         139      230      +91     
==========================================
+ Hits          120      204      +84     
- Misses         19       26       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/utils.rs

zaneduffield · 2024-04-23T20:56:32Z

It seems I used a feature (is_some_and) that was unstable in Rust 1.66 (the MSRV I presume).

Maybe this project could be using the new MSRV clippy lint? I think in theory it would lint a case like this when running on a newer version of rust.

sunshowers · 2024-04-23T21:24:12Z

Maybe this project could be using the new MSRV clippy lint? I think in theory it would lint a case like this when running on a newer version of rust.

I hadn't heard of this! Sure, happy to take a change for it.

sunshowers · 2024-04-23T22:02:40Z

Ahh looks like it's the new incompatible_msrv lint: rust-lang/rust-clippy#12160. But it looks like it's in Rust 1.78 and above, and we're using stable (1.77) to run Clippy against.

sunshowers · 2024-04-23T22:05:46Z

(going by https://rust-lang.github.io/rust-clippy/master/index.html#/incompatible_msrv which says 1.78)

zaneduffield · 2024-04-23T22:47:20Z

Ahh looks like it's the new incompatible_msrv lint: rust-lang/rust-clippy#12160. But it looks like it's in Rust 1.78 and above, and we're using stable (1.77) to run Clippy against.

OK maybe an improvement for the future, then.

sunshowers · 2024-04-23T22:51:07Z

Yeah -- given that it's enabled by default in 1.78, hopefully it'll just happen for free though.

zaneduffield · 2024-04-24T13:38:43Z

I think my new integration test is failing in CI because colour is being forced on in the nextest output. I'll see if I can force it back off.

zaneduffield · 2024-04-24T13:47:53Z

Let me know what you think of the new rudimentary integration test.

sunshowers

Just a few more comments, thanks!

src/runner.rs

tests/run_example.rs

src/runner.rs

tests/run_example.rs

This fixes a quadratic performance issue in which the test directories are iterated for every test case when run under nextest.

sunshowers

Fantastic, thanks!

sunshowers · 2024-04-25T09:00:15Z

Had to do a few more changes:

only check for exact_tests > 1 if the nextest mode is process-per-test: 8bf6f76 (my oversight, should have mentioned this to you)
appease the Windows gods: 56c334a

In any case, this is now out as datatest-stable 0.2.8. Thanks for all your hard work!

zaneduffield · 2024-04-25T12:46:35Z

Had to do a few more changes:

only check for exact_tests > 1 if the nextest mode is process-per-test: 8bf6f76 (my oversight, should have mentioned this to you)

Makes sense

appease the Windows gods: 56c334a

Oops! I should have remembered that : isn't a valid windows file character...

In any case, this is now out as datatest-stable 0.2.8. Thanks for all your hard work!

You're welcome

sunshowers reviewed Apr 23, 2024

View reviewed changes

src/utils.rs Show resolved Hide resolved

zaneduffield force-pushed the runExactTest branch from 9c4ee86 to 3799e6b Compare April 24, 2024 13:36

zaneduffield force-pushed the runExactTest branch from 3799e6b to f31a937 Compare April 24, 2024 13:39

zaneduffield requested a review from sunshowers April 24, 2024 13:47

sunshowers reviewed Apr 24, 2024

View reviewed changes

tests/run_example.rs Show resolved Hide resolved

zaneduffield added 2 commits April 25, 2024 08:51

Avoid iterating files when --exact is passed in

a4a395a

This fixes a quadratic performance issue in which the test directories are iterated for every test case when run under nextest.

Add integration test for cargo nextest run on the example test harness

32942ad

zaneduffield force-pushed the runExactTest branch from f31a937 to 32942ad Compare April 24, 2024 22:52

zaneduffield requested a review from sunshowers April 24, 2024 22:54

sunshowers approved these changes Apr 25, 2024

View reviewed changes

sunshowers merged commit 54f8020 into nextest-rs:main Apr 25, 2024
7 checks passed

zaneduffield deleted the runExactTest branch April 25, 2024 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid iterating files when `--exact` is passed in #49

Avoid iterating files when `--exact` is passed in #49

zaneduffield commented Apr 23, 2024

zaneduffield commented Apr 23, 2024

sunshowers left a comment

sunshowers Apr 23, 2024

zaneduffield Apr 24, 2024

sunshowers Apr 23, 2024

zaneduffield Apr 24, 2024

codecov bot commented Apr 23, 2024 •

edited

Loading

zaneduffield commented Apr 23, 2024

sunshowers commented Apr 23, 2024

sunshowers commented Apr 23, 2024 •

edited

Loading

sunshowers commented Apr 23, 2024

zaneduffield commented Apr 23, 2024

sunshowers commented Apr 23, 2024

zaneduffield commented Apr 24, 2024

zaneduffield commented Apr 24, 2024

sunshowers left a comment

sunshowers left a comment

sunshowers commented Apr 25, 2024

zaneduffield commented Apr 25, 2024

Avoid iterating files when --exact is passed in #49

Avoid iterating files when --exact is passed in #49

Conversation

zaneduffield commented Apr 23, 2024

zaneduffield commented Apr 23, 2024

sunshowers left a comment

Choose a reason for hiding this comment

sunshowers Apr 23, 2024

Choose a reason for hiding this comment

zaneduffield Apr 24, 2024

Choose a reason for hiding this comment

sunshowers Apr 23, 2024

Choose a reason for hiding this comment

zaneduffield Apr 24, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2024 • edited Loading

Codecov Report

zaneduffield commented Apr 23, 2024

sunshowers commented Apr 23, 2024

sunshowers commented Apr 23, 2024 • edited Loading

sunshowers commented Apr 23, 2024

zaneduffield commented Apr 23, 2024

sunshowers commented Apr 23, 2024

zaneduffield commented Apr 24, 2024

zaneduffield commented Apr 24, 2024

sunshowers left a comment

Choose a reason for hiding this comment

sunshowers left a comment

Choose a reason for hiding this comment

sunshowers commented Apr 25, 2024

zaneduffield commented Apr 25, 2024

Avoid iterating files when `--exact` is passed in #49

Avoid iterating files when `--exact` is passed in #49

codecov bot commented Apr 23, 2024 •

edited

Loading

sunshowers commented Apr 23, 2024 •

edited

Loading