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

[Docs]: Document how the "go test ./..." behaves when multiple test binaries are executed #809

Closed
mdelapenya opened this issue Feb 6, 2023 · 9 comments
Labels
documentation Docs, docs, docs. hacktoberfest Pull Requests accepted for Hacktoberfest.

Comments

@mdelapenya
Copy link
Member

Proposal

Context: https://testcontainers.slack.com/archives/C1SUBPZK6/p1674670070385259

go test works creating a test binary for each package if used with ./... . Therefore, each test binary (for each package to be tested) is isolated from the rest packages. Because of that, those test binaries do not share the state of the existing mutexes or any other thing that is synchronised: as it's in the case for the code that reuses or creates the reaper container, or the reuse container feature (we still have to revisit that, reusing the container by request hash).

This behaviour is how go test works, and we should stand with that, not allowing different go packages to access the state created by other packages.

Repro scenario: https://github.com/hahuang65/foo

Thanks to @hahuang65 for the use case and the repro.

@mdelapenya mdelapenya added the documentation Docs, docs, docs. label Feb 6, 2023
@HofmeisterAn
Copy link
Contributor

From my understanding, this is the expected behavior. When tests are run in isolation, each test starts its own instance of the Resource Reaper. This is similar in .NET where, tests distributed across multiple test projects start their own instance of the Resource Reaper. Each test project is treated as a separate test session.

@mdelapenya
Copy link
Member Author

In the case of Go, they are not exactly different test projects but instead different test packages, although treated as different processes as each package will leave in different test binaries. That's why I think we should keep the behaviour and document it.

Apart from that, that has been the default behaviour in the past, and it was never an issue for our users 😅

@hahuang65
Copy link

hahuang65 commented Feb 8, 2023

@mdelapenya first, thank you so much for looking into this, as well as your work on the project. It's enormously useful.

I've deleted my repo (my own OCD, but I'm happy to create it and store it somewhere if you have any suggestions, or think it's worth keeping the reproduction of this issue)I fully appreciate the limitations of how the go test will build separate binaries for each package.

I also fully appreciate and respect that, absolutely, it should not be allowed for different go packages to access state created by other packages.

I'm also speaking here, fully unaware of how the internals of testcontainers-go works with regards to the container reuse mechanism.

However, I will mention, from the point-of-view of a user, there is a disconnect. Reproduction of this "issue" ONLY happens if the container does not already exist. If you run the reproduction a second time, the issue is gone, because the first run stood up the container.

This suggests to me that the reuse mechanism CAN detect if a container is running in Docker. What I'm failing to understand is why it fails when two packages attempt to create the container at roughly the same time?

It seems to me that if a go test run can detect and reuse a container from a previous run, say 10 seconds ago, why shouldn't a go test` run be able to do that if the time difference is just milliseconds ago? The error that appears doesn't seem to suggest a race condition, it suggests that a container with the same name already exists.

EDIT: I'm not writing this to say "THIS MUST BE FIXED", rather, I'm trying to give a little more context around the issue, and ask, as a user, it seems that if this issue doesn't exist across multiple test runs, but only within the same test run, it seems like it shouldn't relate to an internal state issue?

@flowchartsman
Copy link

flowchartsman commented Aug 30, 2023

I have actually run into this as well, and it plays havoc if you are attempting to do anything with networks or if you are giving containers names, which, unless they can by synchronized, breaks re-use. This means you can't easily make a mini "integration framework" for larger projects without sticking everything in one directory.

Depending on whether it's philosophically worth it, you might be able to solve this with lockfiles and an option like "SharedSession", that would take a string and then cause the provider to get wrapped by one that grabs a lockfile named after the container on every ReuseOrCreateContainer, and would also include the reaper. This could avoid contention across test processes.

@mdelapenya
Copy link
Member Author

I have actually run into this as well, and it plays havoc if you are attempting to do anything with networks or if you are giving containers names, which, unless they can by synchronized, breaks re-use. This means you can't easily make a mini "integration framework" for larger projects without sticking everything in one directory.

Depending on whether it's philosophically worth it, you might be able to solve this with lockfiles and an option like "SharedSession", that would take a string and then cause the provider to get wrapped by one that grabs a lockfile named after the container on every ReuseOrCreateContainer, and would also include the reaper. This could avoid contention across test processes.

@flowchartsman do you feel this PR satisfies that? #1513 (do not consider the go.mod|sum files, as they were needed to make modules lint & compile.

In that PR we are defining a SessionID, obtained by the hash of fmt.Sprintf("testcontainers-go:%d:%d", parentPID, parentPIDCreationTime), which is unique per the entire test execution/session: it will give the same string for all the packages in a project.

The PR is still in WIP, but very close to its final review.

@flowchartsman
Copy link

flowchartsman commented Sep 1, 2023

@mdelapenya: This is of great interest to me in the short-term, since I have a plan to migrate the testing strategy of pulsar-client-go to use testcontainers, and I would love to be able to structure it such that tests can use fixtures like integration.Client() and get a client which is set up to talk to the testcontainer, and will create it if necessary. This would be especially great, since it would make sure that only tests which need the container will create it, allowing other tests to be run individually without creating a container. This is a flexible strategy that would benefet any number of packages, I would expect.

Pulsar is a relatively expensive container to spin up, and requires initialization in the form of certs and files being copied in, and I would definitely not want to spin up more than one container, no matter how many tests are being run.

Currently, the pulsar tests are all in a single package, but with the addition of pulsaradmin to the library, this will change shortly, and I plan to start work on the testcontainers migration as part of my work on apache/pulsar-client-go#1085

Please let me know if there is anything I can do to help move this along!

@mdelapenya
Copy link
Member Author

since I have a plan to migrate the testing strategy of pulsar-client-go

@flowchartsman are you aware of our pulsar module? I think it's a great opportunity to use it and improve it if needed.

@flowchartsman
Copy link

flowchartsman commented Sep 4, 2023

I am, and I can submit a PR when it all shakes out, but the testing needs of the official client involve a fair bit of setup with config file fixtures and such and these are not orthogonal to the options already provided by the module. The testing for the client also involves both TLS and non-TLS routes, since both need to be tested, and I imagine with the testcontainers module you'd only want to provide one or the other, depending on how it is configured.

That said, once the changes to the client finally land, I'm happy to issue a PR to add as many of these features as makes sense for testcontainers.

@mdelapenya mdelapenya added the hacktoberfest Pull Requests accepted for Hacktoberfest. label Oct 11, 2023
@mdelapenya
Copy link
Member Author

Even though this issue was originally created for documenting the go test behaviour for multiple packages, I'm going to close this issue as completed because of these two reasons:

Please reopen it if you think it's not done yet 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Docs, docs, docs. hacktoberfest Pull Requests accepted for Hacktoberfest.
Projects
None yet
Development

No branches or pull requests

4 participants