Skip to content

fix(generic): Migrate ServerContainer from deprecated decorator to HttpWaitStrategy#971

Open
jmillxyz wants to merge 2 commits intotestcontainers:mainfrom
jmillxyz:fix/generic-server-wait-strategy
Open

fix(generic): Migrate ServerContainer from deprecated decorator to HttpWaitStrategy#971
jmillxyz wants to merge 2 commits intotestcontainers:mainfrom
jmillxyz:fix/generic-server-wait-strategy

Conversation

@jmillxyz
Copy link
Copy Markdown

Hi there, I've seen this deprecation warning in a generic container. This is my first PR in this project; I'm open to any feedback you may have!

Related: #874

Also updates the docs, highlighted in this comment

@Tranquility2
Copy link
Copy Markdown
Contributor

Hi @jmillxyz thanks for the contribution, can you please rebase and I'll take a look?

@Tranquility2 Tranquility2 changed the title Migrate ServerContainer from deprecated decorator to HttpWaitStrategy fix: Migrate ServerContainer from deprecated decorator to HttpWaitStrategy Apr 1, 2026
@Tranquility2 Tranquility2 changed the title fix: Migrate ServerContainer from deprecated decorator to HttpWaitStrategy fix(generic): Migrate ServerContainer from deprecated decorator to HttpWaitStrategy Apr 1, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.54%. Comparing base (83157eb) to head (6bd25e5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #971   +/-   ##
=======================================
  Coverage   79.54%   79.54%           
=======================================
  Files          15       15           
  Lines        1281     1281           
  Branches      151      151           
=======================================
  Hits         1019     1019           
  Misses        218      218           
  Partials       44       44           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tranquility2 Tranquility2 self-requested a review April 1, 2026 15:05
@wait_container_is_ready(HTTPError, URLError)
def _connect(self) -> None:
# noinspection HttpUrlsUsage
url = self._create_connection_url()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure skipping the _create_connection_url is a good idea

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jmillxyz wdyt?

Copy link
Copy Markdown
Contributor

@Tranquility2 Tranquility2 Apr 1, 2026

Choose a reason for hiding this comment

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

Notice it has some logic

def _create_connection_url(self) -> str:
        if self._container is None:
            raise ContainerStartException("container has not been started")
        host = self.get_container_host_ip()
        exposed_port = self.get_exposed_port(self.internal_port)
        url = f"http://{host}:{exposed_port}"
        return url

@Tranquility2 Tranquility2 self-requested a review April 1, 2026 15:38
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