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

parallel tests #3904

Closed
wants to merge 2 commits into from
Closed

parallel tests #3904

wants to merge 2 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jun 19, 2023

Description

Parallelize tests: seems like a dramatic speed boost

Commit Message / Changelog Entry

perf: parallelize tests

Benchmark

go clean -cache
go clean -testcache
time make test

for me on a 16gb macbook pro m1:

main
make test 564.15s user 84.38s system 295% cpu 3:39.64 total

parallel-tests
make test 560.54s user 83.24s system 296% cpu 3:37.23 total

Interested to see if this runs faster for those with faster machines though.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see
    CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@faddat
Copy link
Contributor Author

faddat commented Jun 19, 2023

I think this might be...slower

At least if you clear the cache first?

Bizarre but I think true

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jun 20, 2023

I think this might be...slower

Yea IIRC the way tests are run via t.Parallel is not as clear cut as one would expect. Non-parallel tests are executed as they are found while parallel tests are paused, collected in some way and then executed all together in parallel in the end (roughly, docs). Implication being if you have mixed parallel/sequential, you probably won't be seeing full benefits.

Note that I am also not sure about the implications of calling parallel on the suite level (i.e suite.Run(t, new(...)) functions), I'm seeing an issue opened since 2016 on testify for this but don't have the time atm to go through it and see how much it might affect us.

In general I'm pretty okay-ish with having tests within a package execute sequentially. Different packages already get tested in parallel so, locally at least, tests finish in around 30seconds for me.

@charleenfei
Copy link
Contributor

thanks for this idea @faddat! however, the tests run pretty quickly for me, i guess i feel that having the additional t.Parallel calls is not necessarily worth the benefit in this case.

in general, i'd prefer to minimise the amount of code that we "need to write" unless the benefit is clear, just in the interest of maintainability.

@faddat
Copy link
Contributor Author

faddat commented Jun 21, 2023

@charleenfei I actually reached the same conclusion that you did. I kind of investigated this some on Twitter and I had other people try it out to, but in the end I'm really not sure that the benefit here is worth the additional lines of code, it's exactly like yourself. So I'm going to leave this open for maybe like one more day and unless somebody drops in saying that it's massively faster I'm going to close it tomorrow 🙂

@faddat faddat closed this Jun 21, 2023
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