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

fix(testcontainers): new port-forwarder container per test file execution #678

Closed
wants to merge 1 commit into from
Closed

fix(testcontainers): new port-forwarder container per test file execution #678

wants to merge 1 commit into from

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Nov 21, 2023

Context

From Jest documentation:

image

Source: https://jestjs.io/docs/configuration#resetmodules-boolean

Since each test file gets its independent module registry (i.e., independent "static" JS module state), the PortForwarderInstance.instance singleton field is undefined when a test file starts the execution. This causes the PortForwarderInstance class to believe that no other port-forwarder container is running and tries to start a new one with the same name.

Proposed solution

Note

This PR fixes the limitation found in #677

A sub-optimal but working solution is to spin up a port-forwarder container per test file with a unique random name. This avoids the container name clash issue reported in #677.

Copy link

netlify bot commented Nov 21, 2023

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 195897b
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/655e40dced9cfe0008294562
😎 Deploy Preview https://deploy-preview-678--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@LNSD
Copy link
Contributor Author

LNSD commented Nov 21, 2023

cc @javierlopezdeancos @mdelapenya

@javierlopezdeancos
Copy link
Contributor

Hey, thanks @LNSD to push this suggest, we will talk about it tomorrow.

Comment on lines +57 to 62
const portForwarderId = new RandomUuid().nextUuid();
const container = await new GenericContainer(SSHD_IMAGE)
.withName(`testcontainers-port-forwarder-${reaper.sessionId}`)
.withName(`testcontainers-port-forwarder-${portForwarderId}`)
.withExposedPorts(containerPort)
.withEnvironment({ PASSWORD: password })
.withCommand([
Copy link
Contributor Author

@LNSD LNSD Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, if having a container name that starts with testcontainers-port-forwarder- is not important, removing the .withName(...) statement would have a similar effect: When the Container engine creates the container instance, it will give it a random name that won't clash with other existing containers.

Suggested change
const portForwarderId = new RandomUuid().nextUuid();
const container = await new GenericContainer(SSHD_IMAGE)
.withName(`testcontainers-port-forwarder-${reaper.sessionId}`)
.withName(`testcontainers-port-forwarder-${portForwarderId}`)
.withExposedPorts(containerPort)
.withEnvironment({ PASSWORD: password })
.withCommand([
const container = await new GenericContainer(SSHD_IMAGE)
.withName(`testcontainers-port-forwarder-${reaper.sessionId}`)
.withExposedPorts(containerPort)
.withEnvironment({ PASSWORD: password })
.withCommand([

@cristianrgreco
Copy link
Collaborator

Hey @LNSD, there is some discussion about the solution in the linked issue #677. This PR or the suggestion to remove the name could result in several PortForwarder containers running for a given test suite which would create its own issues

@LNSD
Copy link
Contributor Author

LNSD commented Nov 24, 2023

Hey @LNSD, there is some discussion about the solution in the linked issue #677. This PR or the suggestion to remove the name could result in several PortForwarder containers running for a given test suite which would create its own issues

Sure. This is, yet suboptimal (due to the multiple containers), a possible solution.

IIRC, testcontainers-java does something similar (random name by docker).

But, please, feel free to close this PR if you find a more suitable solution, or reach me via slack if you want to discuss anything) 🙂

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