Skip to content

Avoid loading compose info unless needed #1001

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 1 commit into from

Conversation

henrinormak
Copy link
Contributor

This improves performance in scenarios where the caller does not rely or need compose in any way.

Anecdotal evidence is from a repository that has 289 test cases, ~260 of which somehow interact with testcontainers. There's enough abstraction between the tests and testcontainers so that not each of the tests hits this slow path, but still, the timing (run on a M3 Pro) shows a clear improvement.

Before - 19.69s, 18.30s, 18.05s, 17.54s, 17.70s - average 18.256s
After - 18.03s, 15.61s, 15.57s, 16.46s, 15.54s - average 16.242s

It's not much, but in all likelihood, depending on the setup, the difference is bigger with more parallel test suites, or situations in which getContainerRuntimeClient is called from different workers/processes more.

Copy link

netlify bot commented May 13, 2025

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 13f5d03
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/68233af8d465f600086c1538
😎 Deploy Preview https://deploy-preview-1001--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.

@henrinormak
Copy link
Contributor Author

What I don't know is how breaking the lack of compose info on the container runtime client is, i.e that's an exposed API from testcontainers that someone could theoretically be depending on.

@henrinormak
Copy link
Contributor Author

@cristianrgreco any feedback on this? I know that it's a bit out of the left-field (unexpected PR or something), but I've been going after any low-hanging fruit in our CI pipelines to optimize testing and this seemed like a good candidate.

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented May 21, 2025

Hey @henrinormak, apologies for the delay. I've seen the PR but not sure what to do with it yet 😄

I agree the time save is worth it, I just don't like introducing the LazyComposeClient. We only need it because currently we need the version to determine whether to use docker-compose v1 or v2 (and not have to await composeClient everywhere). The thing is, as part of this PR #938, we're dropping support for docker-compose v1. In that PR we still log the version (and so have this performance hit), but it could be as simple as removing it, as done in this PR, 2 line change. I'm tempted to leave this PR and drop the version log in #938. WDYT?

@cristianrgreco
Copy link
Collaborator

Superseded by #1012

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.

2 participants