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

Improve unit test speed #6111

Open
yurishkuro opened this issue Oct 22, 2024 · 6 comments
Open

Improve unit test speed #6111

yurishkuro opened this issue Oct 22, 2024 · 6 comments
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement performance

Comments

@yurishkuro
Copy link
Member

This blog post provides a good background / introduction: https://threedots.tech/post/go-test-parallelism/

When I ran the following command

GOMAXPROCS=1 go test -parallel 128 -p 16 -json ./... | go run github.com/roblaszczak/vgt@latest

I got the following output:

Image

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 adding t.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.

@yurishkuro yurishkuro added good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement labels Oct 22, 2024
@dosubot dosubot bot added the performance label Oct 22, 2024
@codesmith25103
Copy link

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

@codesmith25103
Copy link

"Hey @yurishkuro, I added t.parallel() to the test function that was using ephemeral ports and got the following result."

Does that look right?

Image

@mmorel-35
Copy link
Contributor

I just found this https://golangci-lint.run/usage/linters/#paralleltest
I hope it can help

@yurishkuro
Copy link
Member Author

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.

@vaidikcode
Copy link
Contributor

Hello @yurishkuro ,
Some observations:
Usage of t.Parallel(): I've noticed that adding t.Parallel() generally only makes sense in tests with blocking operations, and even then, the impact on overall test speed can be minimal for some cases.
Impact on Test Tables: The most significant improvement in test speed has been within test tables.
I was able to reduce the test time for the crossdock/services package from 2.859 seconds to 2.760 seconds.
Given these results, I’d like to know if you’d like me to continue refining this package or focus on specific areas. Additionally, do we have a target or expected test time reduction for each package? Knowing this will help me prioritize and make the best use of parallelism where it counts.

@yurishkuro
Copy link
Member Author

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.

yurishkuro pushed a commit that referenced this issue Nov 11, 2024
## 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants