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

Documentation for StartupCheckStrategy includes non-working code snippet #754

Open
nick-jones opened this issue Apr 11, 2024 · 1 comment
Labels
documentation Changes to documentation

Comments

@nick-jones
Copy link

Expected Behaviour
https://github.com/testcontainers/testcontainers-node/blob/main/docs/features/wait-strategies.md#other-startup-strategies shows a snippet of code which demonstrates how custom wait strategies can be implemented.

This code is not valid after c97e2942f047814a5d80835402cf2fcf3d944913 introduced a constructor to StartupCheckStrategy, with the expectation that ContainerRuntimeClient is injected.

I would think something like this would perhaps be more suitable?

const client = await getContainerRuntimeClient();
const container = await new GenericContainer("alpine")
  .withWaitStrategy(new ReadyAfterDelayWaitStrategy(client))
  .start();

Actual Behaviour
If I follow the documentation as suggested, I'll get an error, since the client is not injected:

TypeError: Cannot read properties of undefined (reading 'container')
    at common_1.IntervalRetry.retryUntil.message (.../node_modules/testcontainers/build/wait-strategies/startup-check-strategy.js:13:134)
    at IntervalRetry.retryUntil (.../node_modules/testcontainers/build/common/retry.js:26:28)
    at ReadyAfterDelayWaitStrategy.waitUntilReady (.../node_modules/testcontainers/build/wait-strategies/startup-check-strategy.js:13:70)
    at waitForContainer (.../node_modules/testcontainers/build/wait-strategies/wait-for-container.js:8:28)
    at GenericContainer.startContainer (.../node_modules/testcontainers/build/generic-container/generic-container.js:140:57)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async .../test.js:15:21

Testcontainer Logs
N/A

Steps to Reproduce
N/A

Environment Information
N/A

@cristianrgreco
Copy link
Collaborator

Thanks for pointing this out. IMO it makes sense to default the constructor property as it's always gonna be getContainerRuntimeClient() , then we don't have to update the docs. Would you be willing to raise this PR?

@cristianrgreco cristianrgreco added the documentation Changes to documentation label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes to documentation
Projects
None yet
Development

No branches or pull requests

2 participants