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

Add tests for single threaded CLI 4GB @ all strategies to trigger index reduction #2601

Closed
wants to merge 1 commit into from

Conversation

senhuang42
Copy link
Contributor

@senhuang42 senhuang42 commented May 3, 2021

#2598 fixes a bug with rowhash that gets large enough to trigger index reduction.

Here, we add some CLI tests that would have trivially caught this (with ASAN). We add a ST 4GB roundtrip test for each strategy, so that future modifications to other strategies will also get tested with huge files.

We also add a ubsan-asan gh actions test.

Test Plan:

@Cyan4973
Copy link
Contributor

Cyan4973 commented May 3, 2021

While this adds some much needed test coverage,
it also seems to weight down CI tests in important ways.

Longest test detected in this run :

generic-dev / test (pull_request) Successful in 51m

which I presume is related to large files single-threaded tests across all strategies.
In contrast, without this new set of tests, the same test seems be shorter. For example, in #2598 :

generic-dev / test (pull_request) Successful in 24m

and in #2597:

generic-dev / test (pull_request) Successful in 24m

So that's not a one-off, and it's far more than 3 minutes.
On top of that, it's making one of our longest tests even longer, thus worsening feedback signal delay.
I don't know what's the time-out limit of Github Actions, but it's getting dangerously close.

Finally, the new test asan-ubsan-testzstd fails, which I presume is expected,
but the important point here is that this premature exit prevents us from measuring the final time spent into the new section large files single-threaded tests across all strategies, since it bails out as soon as the error is triggered.
I would expect this test to be far slower than just make test, without uasan, which we just mentioned doubled in test time spent to reach something close to an hour.

This is a question of balance.
We need better test coverage, but not at the cost of everything else,
and not at the cost of significantly worse CI feedback loop.

Try to find ways to make the feedback loop less impacted.
One possibility here would be to separate the test, so that it's not part of make test, which is run a lot of time already.
Possibly, split the test across VM, so that each one of them remain <= ~20mn limit if possible.

@senhuang42
Copy link
Contributor Author

senhuang42 commented May 3, 2021

Possibly, split the test across VM, so that each one of them remain <= ~20mn limit if possible.

Ah interesting. I'm guessing we can just add more gh actions that basically just each perform the action:
datagen -g4000M -P99 | zstd -v --zstd=strategy=1 --single-thread | zstd -d --zstd=strategy=1 --single-thread, rather than adding this to make test. Though that does mean that the test only exists in CI, and not in local tests.

Edit: still looks like it takes too long

@senhuang42 senhuang42 force-pushed the rowhash_cli_test branch 2 times, most recently from 5245b36 to 102074d Compare May 3, 2021 21:39
@terrelln
Copy link
Contributor

terrelln commented May 3, 2021

We might be able to abandon this in favor of #2603.

@senhuang42
Copy link
Contributor Author

We might be able to abandon this in favor of #2603.

Agreed, this would be redundant.

@senhuang42 senhuang42 closed this May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants