-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve log messages when database container test query fails #3015
Improve log messages when database container test query fails #3015
Conversation
@@ -867,7 +868,6 @@ public void setWaitStrategy(org.testcontainers.containers.wait.strategy.WaitStra | |||
* @see #waitingFor(org.testcontainers.containers.wait.strategy.WaitStrategy) | |||
*/ | |||
protected void waitUntilContainerStarted() { | |||
org.testcontainers.containers.wait.strategy.WaitStrategy waitStrategy = getWaitStrategy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure if it was made on purpose. The local variable hides the instance variable and seemed redundant.
@vcvitaly could you please revert changes unrelated to the MySQL message issue? Feel free to submit them separately, but this PR should have the bare minimum to verify the fix. |
Also, I wasn't sure if I have to write a test and how. Although I'd like to, I haven't found a way to simulate a delay. |
The fix also depends on #3018 since the logging is not working in the current implementation for some log levels. |
could you please clarify what exactly does not work? Re testing, you can override mysql's command and make it sleep for a second or two before calling |
The exception I throw from
But since logger name provided by DockerLoggerFactory is effectively "${dockerImageName}" it won't fall under "org.testcontainers" logger but rather under root where level is "INFO". An exception will be thrown further and container startup will fail but the exact reason won't be printed.
|
@bsideup @rnorth @kiview The problem is that I need to get some values from the waitStrategyTarget in order to perform verification. Would it make sense to parametrize the AbstractWaitStrategy? |
You are correct with your observation and this implementation is there for legacy reasons. You are also right that this should be tackled in the future, however, this is not necessarily something to take on lightly (this would probably be an API breaking change) and you should not do this as part of this PR. |
Ok, is that something I could try doing in a separate PR or shall I leave it for now? |
I think it is better to leave this now since I don't think reviewing such a big PR would have a high priority for the core-maintainer team atm and as I said, I see quite some risk in breaking our public API there. |
@bsideup I wrote a test for a minimal implementation, please review. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Show resolved
Hide resolved
modules/jdbc/src/test/java/org/testcontainers/containers/JdbcDatabaseContainerTest.java
Outdated
Show resolved
Hide resolved
modules/jdbc/src/test/java/org/testcontainers/containers/JdbcDatabaseContainerTest.java
Outdated
Show resolved
Hide resolved
@vcvitaly ping |
Co-authored-by: Sergei Egorov <bsideup@gmail.com>
@bsideup Ping :) |
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
Outdated
Show resolved
Hide resolved
…er_start_timeout # Conflicts: # modules/jdbc/build.gradle
Merged. Thank you for your extreme patience, @vcvitaly 🙇♂️ |
Throw an exception if container is started but test sql query cannot be executed (cannot connect to database port or smth else).
Fixes #2878