Skip to content

feat: implement .withAutoRemove #850 #905

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

Closed
wants to merge 2 commits into from

Conversation

botflux
Copy link
Contributor

@botflux botflux commented Jan 27, 2025

Hey, here is a PR to implement the request of #850.
I've implemented the following behavior:

  • .withAutoRemove(false) disables container's auto removal
  • .stop({ remove: true }) overrides the .withAutoRemove(false), so the container is removed
  • By default, containers are automatically removed

I've also added sections about the new behaviors close to existing sections about .stop({ remove: false }).

On the other hand, I've noted that docker prefixes container name with a slash, meaning that the test I wrote would fail:

  it("should stop but not remove the container", async () => {
    const container = await new GenericContainer("cristianrgreco/testcontainer:1.1.14")
      .withName(`container-${new RandomUuid().nextUuid()}`)
      .withAutoRemove(false)
      .start();

    await container.stop();

    expect(await getRunningContainerNames()).not.toContain(container.getName()); // never fails because container is prefixed
    expect(await getStoppedContainerNames()).toContain(container.getName()); // fails because `container.getName()`' result is prefixed with '/'.
  });

Here is the output of jest:

Summary of all failing tests
 FAIL  packages/testcontainers/src/generic-container/generic-container.test.ts (90.581 s)
  ● GenericContainer › should stop but not remove the container

    expect(received).toContain(expected) // indexOf

    Expected value: "/container-4e957eea11e6"
    Received array: ["container-4e957eea11e6", "testcontainers-22930a5025fc-service_2-1", "testcontainers-686a91976fd7-service_1-1", "testcontainers-686a91976fd7-service_2-1", "container-797b0e580f81", "testcontainers-a1999481affa-service_2-1", "testcontainers-39d1bd573914-service_1-1", "testcontainers-39d1bd573914-service_2-1", "goofy_aryabhata", "silly_hofstadter", …]

      530 |
      531 |     expect(await getRunningContainerNames()).not.toContain(container.getName());
    > 532 |     expect(await getStoppedContainerNames()).toContain(container.getName());
          |                                              ^
      533 |   });
      534 |
      535 |   it("should stop and override .withAutoRemove", async () => {

      at Object.<anonymous> (src/generic-container/generic-container.test.ts:532:46)


Test Suites: 1 failed, 44 passed, 45 total
Tests:       1 failed, 3 skipped, 323 passed, 327 total
Snapshots:   0 total
Time:        114.873 s
Ran all test suites.

To fix the test I've removed the leading '/'

    const container = await new GenericContainer("cristianrgreco/testcontainer:1.1.14")
      .withName(`container-${new RandomUuid().nextUuid()}`)
      .withAutoRemove(false)
      .start();

    await container.stop();

    expect(await getRunningContainerNames()).not.toContain(container.getName().replace("/", ""));
    expect(await getStoppedContainerNames()).toContain(container.getName().replace("/", "")); // passes

This feels more like a workaround than a proper solution, and I'm open to explanations/better solutions.

Copy link

netlify bot commented Jan 27, 2025

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit e413d81
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/67d8a15a886398000861f09f
😎 Deploy Preview https://deploy-preview-905--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.

@cristianrgreco cristianrgreco added enhancement New feature or request minor Backward compatible functionality labels Jan 31, 2025
digital88 added a commit to digital88/testcontainers-node that referenced this pull request Mar 18, 2025
cristianrgreco pushed a commit that referenced this pull request Mar 19, 2025
@cristianrgreco
Copy link
Collaborator

Replaced by #939

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants