-
Notifications
You must be signed in to change notification settings - Fork 21
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
Refactor and parallelize integration tests #216
Refactor and parallelize integration tests #216
Conversation
@tamirms the PR is pretty big, but I have created self-container commits which should be reviewable incrementally. |
@tamirms It's still a draft until we check how it performs and I can address a couple of TODOs. |
cmd/soroban-rpc/internal/integrationtest/infrastructure/test.go
Dismissed
Show dismissed
Hide dismissed
cmd/soroban-rpc/internal/integrationtest/infrastructure/test.go
Dismissed
Show dismissed
Hide dismissed
It turns out that the tests are around 3 times faster in CI (I will try to play with the parallelism level to see if it improves) |
I get spurious port clashes:
Probably the issue is that you cannot really reserve ports with 100% accuracy (the only way to make sure you reserve them is by using them right away) |
@2opremio you can use this approach to let docker-compose select open ports on the host for the exposed ports on the containers: https://stackoverflow.com/questions/60109556/expose-random-port-to-docker-compose-yml there is a docker-compose command you can use to find the port mapping between host and container |
Ahh, interesting. I will look into that. That should reduce the clashes (or even completely remove them) |
cmd/soroban-rpc/internal/integrationtest/infrastructure/test.go
Dismissed
Show dismissed
Hide dismissed
195881e
to
44677be
Compare
@tamirms I applied your suggestion. The only preallocated port is Captive Core's peer port, because there doesn't seem to be a way to make Core allocate it dynamically. We can even get rpc to use dynamic ports! :) |
Now the problem is I cannot get an ordered output of the failing tests (since tests are running in parallel, golang outputs a mixed output) which makes it hard to see what went wrong. |
The logging should be fixed. This blogpost was pretty useful: https://brandur.org/t-parallel The trick was to use |
Tests are now failing some times because of stellar/go#5350 |
I fixed the race at stellar/go#5352 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a lot to take in so my review isn't super comprehensive but I do have a main question: can you control "how parallel" the tests are or does it run them all at once? Gonna try to run them locally and I'm worried it won't work on older machines.
Edit: ah, your linked post explains you can use -count=N
for it.
cmd/soroban-rpc/internal/integrationtest/infrastructure/client.go
Outdated
Show resolved
Hide resolved
Co-authored-by: George <[email protected]>
@Shaptic PTAL |
It’s
|
What
Parallelize and refactor integration tests.
Why
Tests look way cleaner and run 10x faster (at least in my local machine)
Followup to #207
Known limitations
The dynamic port allocation makes the code a little less readable