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

test: speed up unit tests by using smaller datasets #563

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

fengelniederhammer
Copy link
Contributor

part of #560

Summary

Local results (debug build):

  • database.test.cpp: Use a smaller dataset. 7s -> 800ms
  • preprocessor.test.cpp: The RSV dataset took 45s -> shift it to the new e2e tests and delete here
  • preprocessor.test.cpp: Use smaller datasets for some tests: 1.8s -> ~500ms

On CI, time of the unit tests:

  • last run on main: 16s
  • first build on this branch: 2s

PR Checklist

- [ ] All necessary documentation has been adapted or there is an issue to do so.

  • The implemented feature is covered by an appropriate test.

Copy link
Contributor

This is a preview of the changelog of the next release:

0.2.18 (2024-09-11)

Features

  • do not abort on assertment failures (4e5f598)
  • panic: add ASSERT, UNREACHABLE, UNIMPLEMENTED, safer env var (867d08f)

* database.test.cpp: Use a smaller dataset. Speed up locally: 7s -> 800ms
* preprocessor.test.cpp: The RSV dataset took 45s -> shift it to the new e2e tests
* preprocessor.test.cpp: Use smaller datasets for some tests: 1.8s -> ~500ms
@Taepper
Copy link
Collaborator

Taepper commented Sep 11, 2024

I added GenSpectrum/LAPIS-SILO-e2e#34 to not drop our covered input data

@fengelniederhammer
Copy link
Contributor Author

I added GenSpectrum/LAPIS-SILO-e2e#34 to not drop our covered input data

IMO the dataset is not of immediate importance. In these tests here, it was just some dataset. We already have some other dataset in other tests.
In the longterm of course still good to have.

@Taepper
Copy link
Collaborator

Taepper commented Sep 11, 2024

Exactly. I just wanted to have the issue such that we remember adding it at some point. Not blocking in any way :)

Copy link
Contributor

@pflanze pflanze left a comment

Choose a reason for hiding this comment

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

Cool. I haven't looked into all of the details, but looks good to me.

(It's good to have a test suite that covers most of the potential issues when coding, and then have other more expensive tests/test suites that give more certainty for a higher cost. Even in unit tests, could have expensive ones if there are 2 classes of tests, important-but-quick and more-extensive-but-expensive ones.)

@fengelniederhammer fengelniederhammer merged commit 617fc8f into main Sep 11, 2024
10 checks passed
@fengelniederhammer fengelniederhammer deleted the speedUpUnitTests branch September 11, 2024 15:00
@Taepper
Copy link
Collaborator

Taepper commented Sep 11, 2024

We could also learn from other codebases. e.g. duckdb marks suffixes some test-files with .test_slow:
https://github.com/duckdb/duckdb/tree/main/test/sql/copy

@Taepper
Copy link
Collaborator

Taepper commented Sep 11, 2024

And then their test runner can execute them based on the arguments given by the developer running the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants