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

Refactor and parallelize integration tests #216

Merged
merged 15 commits into from
Jun 18, 2024

Conversation

2opremio
Copy link
Contributor

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

@2opremio 2opremio requested a review from tamirms June 14, 2024 10:54
@2opremio
Copy link
Contributor Author

@tamirms the PR is pretty big, but I have created self-container commits which should be reviewable incrementally.

@2opremio
Copy link
Contributor Author

@tamirms It's still a draft until we check how it performs and I can address a couple of TODOs.

@2opremio
Copy link
Contributor Author

2opremio commented Jun 14, 2024

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)

@2opremio 2opremio marked this pull request as ready for review June 14, 2024 13:54
@2opremio
Copy link
Contributor Author

I get spurious port clashes:

Error response from daemon: driver failed programming external connectivity on endpoint testgetnetworksucceeds-core-1 (b49f38b68105bcc50def3377640097ea93dac08deb115d612624fd4896bace26): Error starting userland proxy: listen tcp4 0.0.0.0:35855: bind: address already in use

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)

@tamirms
Copy link
Contributor

tamirms commented Jun 14, 2024

@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

@2opremio
Copy link
Contributor Author

2opremio commented Jun 14, 2024

Ahh, interesting. I will look into that. That should reduce the clashes (or even completely remove them)

@2opremio 2opremio force-pushed the refactor-and-parallelize-itests branch from 195881e to 44677be Compare June 16, 2024 15:51
@2opremio
Copy link
Contributor Author

@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! :)

@2opremio
Copy link
Contributor Author

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.

@2opremio
Copy link
Contributor Author

The logging should be fixed. This blogpost was pretty useful: https://brandur.org/t-parallel

The trick was to use testing.T.Log() for everything

@2opremio
Copy link
Contributor Author

Tests are now failing some times because of stellar/go#5350

@2opremio
Copy link
Contributor Author

I fixed the race at stellar/go#5352

Copy link
Contributor

@Shaptic Shaptic left a 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.

Co-authored-by: George <Shaptic@users.noreply.github.com>
@2opremio
Copy link
Contributor Author

@Shaptic PTAL

@2opremio
Copy link
Contributor Author

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.

It’s -parallel, for which Go chooses a reasonable default. See https://pkg.go.dev/cmd/go/internal/test

By default, -parallel is set to the value of GOMAXPROCS.

@2opremio 2opremio enabled auto-merge (squash) June 18, 2024 17:42
@2opremio 2opremio merged commit 3d2810f into stellar:main Jun 18, 2024
19 checks passed
@2opremio 2opremio deleted the refactor-and-parallelize-itests branch June 18, 2024 18:00
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