Skip to content

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.

@netlify
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