-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve unit test speed #6111
Comments
Hey @yurishkuro. I wish to work on this issue. Although I am new to open source but I am eager to contribute in Jaeger. I have already setup code in my local machine |
"Hey @yurishkuro, I added t.parallel() to the test function that was using ephemeral ports and got the following result." Does that look right? |
I just found this https://golangci-lint.run/usage/linters/#paralleltest |
I'd be careful with that linter - not all tests benefit from parallel, it can often make things worse. I'd rather treat this as performance optimization problem - measure first, fix where it makes a difference. |
Hello @yurishkuro , |
In the issue description there was a link to a blog post that explains how to optimize total time by looking at the trace diagram. Going from 2.8s to 2.7s is not worth the effort. |
## Which problem is this PR solving? - Part of #6111 ## Description of the changes - Added t.parallel to tests where it made significant improvement ## How was this change tested? - ## Checklist - [ ] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [ ] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: chahatsagarmain <[email protected]>
This blog post provides a good background / introduction: https://threedots.tech/post/go-test-parallelism/
When I ran the following command
I got the following output:
It shows some tests running for a very long time (the darker blue colors), but even of some of them were sped up there are others (like in the first group in top left corner) that are still relatively long.
We also almost never use
t.Parallel()
in our test (right now I only found one instance), even though many of our tests are starting servers and making network calls, so could benefit from parallelism. But addingt.Parallel()
should be done with care - some tests are starting servers on the same port (instead of ephemeral port) and must be refactored before adding parallelism.The text was updated successfully, but these errors were encountered: