search

testcontainers / testcontainers-python

Testcontainers is a Python library that providing a friendly API to run Docker container. It is designed to create runtime environment to use during your automatic tests.
https://testcontainers-python.readthedocs.io/en/latest/
Apache License 2.0
1.58k stars 288 forks source link

fix: wait_for_logs can now fail early when the container stops #682

Closed ArthurBook closed 2 months ago

ArthurBook commented 2 months ago

Addresses my suggestion made in issue 681.

This PR adds a flag that checks is the status is not running and raises a RuntimeError to avoid waiting for logs after the container already has exited. The idea is to save wait time when there is a long startup time in case the container fails early.

from testcontainers.core import container, waiting_utils

if __name__ == "__main__":
    waiting_utils.wait_for_logs(
        container.DockerContainer("flyway/flyway").start(),
        r"Successfully applied \d+ migrations to schema",
        timeout=10,
        raise_on_exit=True,
    )

# > RuntimeError(f"Container exited before emitting logs satisfying predicate")
# ( Raised almost immediately )
codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@c7d9b81). Learn more about missing BASE report.

Files Patch % Lines
core/testcontainers/core/waiting_utils.py 0.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #682 +/- ## ======================================= Coverage ? 78.07% ======================================= Files ? 12 Lines ? 602 Branches ? 89 ======================================= Hits ? 470 Misses ? 106 Partials ? 26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alexanderankin commented 2 months ago

why is true is not the default, any background on this?

ArthurBook commented 2 months ago

why is true is not the default, any background on this?

I think it would be a good default. Kept it False just to not change existing functionality without explicit opt-in.

alexanderankin commented 2 months ago

i would like to roll it out everywhere else, wonder what would be a good strategy for that, unless i am overthinking it, in which case we should just do it.

kiview commented 2 months ago

TBH, I think the false behavior is more a bug than actual expected behavior. If I were to make the call, I would not only make true the default, but also don't make it user configurable at all (can any of you come up with a use case were the false behavior is useful?).

alexanderankin commented 2 months ago

pr title = "fix: wait_for_logs can now fail early when the container stops"

next pr title = ${pr title/can/WILL/}

ArthurBook commented 2 months ago

Hi! Agree with above, for all our use cases, the expected behavior would be to raise RuntimeError if the container exited. I think its a good idea to drop the raise_on_exit param and fix behavior to True.

Another option is to allow user to define the set of Container.status that (don't) cause a fail. This would allow for a bit more configurability. WDYT?