Add LogMessageWaitStrategy #322

kiview · 2017-04-03T22:46:31Z

Added a LogMessageWaitStrategy which checks, if a log message contains the specified String. This might help in fixing #317 when incorporating it into PostgreSQLContainer in the future.

bsideup · 2017-04-04T07:17:47Z

core/src/test/java/org/testcontainers/junit/wait/LogMessageWaitStrategyTest.java

+
+    @Test
+    public void testWaitUntilReady_Success() {
+        waitUntilReadyAndSucceed("while true; do sleep 1; echo -e \"" + READY_MESSAGE + "\"; done");


I would avoid sleep here because it adds a static delay to the test execution, how about "print & infinite sleep" instead?

I wanted a delay between the prints to avoid excessive printing, your version sounds better though.

bsideup · 2017-04-04T07:22:12Z

core/src/main/java/org/testcontainers/containers/wait/LogMessageWaitStrategy.java

+        container.followOutput(waitingConsumer);
+
+        Predicate<OutputFrame> waitPredicate = outputFrame ->
+                outputFrame.getUtf8String().contains(expectedLogPart);


I'm wondering if simple contains() is enough or not... Maybe some pattern based solution would be better?

At first I wanted to expose a method forPredicate(), but I think something like OutputFrame shouldn't be part of the public API?

WDYT about using matches() instead?

I'd imagine an overloaded withExpectedLogPart method that accepts a Pattern could be flexible enough, couldn't it?

bsideup · 2017-04-04T07:29:58Z

core/src/main/java/org/testcontainers/containers/wait/LogMessageWaitStrategy.java

+    @Override
+    protected void waitUntilReady() {
+        WaitingConsumer waitingConsumer = new WaitingConsumer();
+        container.followOutput(waitingConsumer);


@rnorth correct me if I'm wrong, but I think here will be a race because WaitingConsumer doesn't use .since(0) and if container prints something immediately after the launch then this code will fail. WDYT?

I think you're right. The WaitingConsumer will start following output after the container was started and will only receive new frames.

Ah yes, I think so. Just thinking, would there be any reason not to amend WaitingConsumer to use since(0)? I can't think of anything off the top of my head...

@rnorth sounds good to me 👍 I would even expect such behavior :)

Should I do the change in this PR then?

So I did a change, but I'm not entirely sure if this was the code you were thinking about ;)
I also wonder if this is really necessary, since the Javadoc says

* @param since * - UNIX timestamp (integer) to filter logs. Specifying a timestamp will only output log-entries since that timestamp. Default: * 0 (unfiltered)

Ah, good. Should be no need then.

Or we keep the change, in order to document the intent more clearly. I'm fine with either way.

Sure, let's keep the withSince(0) there now that it's in - no harm, and like you say, it documents the intent.

kiview · 2017-04-07T15:03:27Z

The failing codacy check is because of an underscore inside the test method BTW, but I wanted the test class to look similar to the existing WaitStrategy tests.

bsideup · 2017-04-07T17:02:44Z

LGTM

/cc @rnorth

kiview · 2017-04-07T17:24:46Z

Regarding the problem with postgres #317, the LogWaitStrategy might need to cater for more complex log output (like logline sequence?). Wouldn't really be happy to introduce this kind of complexity though TBH.

kiview · 2017-04-08T08:07:15Z

I'd like to implement multi line matching in WaitingConsumer before merging the PR.

rnorth · 2017-04-09T06:08:54Z

How shall we go about implementing the multi-line matching? A few options spring to mind:

Add times as a parameter to WaitingConsumer methods, so that the predicate has to be matched multiple times. This would be simple, and clearly works for the docker-maven-plugin team.
Allow Predicates to be used that allow the entire LinkedBlockingDeque<OutputFrame> to be looked at. However, Deque doesn't lend itself to performant traversal..
Do something around composing a chain of Predicates. Perhaps this is something that already exists in a library. This feels like an interesting exercise otherwise, but perhaps a bit yak-shavy.

Personally I think the first option is probably good enough for now, and we could make it more sophisticated if/when we find a compelling need.

Or maybe there's another way?

WDYT?

kiview · 2017-04-09T10:21:42Z

@rnorth AFAIK the docker-maven-plugin keeps on continuously building one single multi line String, using a StringBuilder and matching against the resulting String. In the PostgreSQL case, this allows them to match for

"(?s)ready to accept connections.*ready to accept connections"

.

I think this is nice from an API user perspective, but it doesn't work that well with the current OutputFrame model.

Would it be okay to switch from using LinkedBlockingDeque to a normal List? I think performance and memory implications shouldn't be so bad, especially since we are inside a testing context.

On the other hand, regarding your proposed solutions, I'd agree with implementing the times based solution first, since it's really easy and serves our current needs. We are working around non optimal implemented container images after all and it should be sufficient to be pragmatic at the moment.

kiview · 2017-04-10T19:10:17Z

I've implemented a solution by adding a times parameter to the WaitingConsumer method. IMO it's enough for the moment.

rnorth · 2017-04-10T19:33:44Z

This looks good to me - thanks for this!
I'll go ahead and merge into master, and aim to release in the next couple of days.

kiview · 2017-04-10T20:34:24Z

I'll look into fixing #317 tomorrow by using WaitingConsumer, might work without bigger refactorings.

Add LogMessageWaitStrategy

99a436e

kiview force-pushed the waitForLog branch from 2c48261 to 99a436e Compare April 3, 2017 22:51

bsideup requested changes Apr 4, 2017

View reviewed changes

bsideup added the enhancement label Apr 4, 2017

bsideup added this to the 1.3.0 milestone Apr 4, 2017

bsideup assigned kiview Apr 4, 2017

kiview added 3 commits April 5, 2017 22:44

Sleep after logging in test, instead of loop

35c6fb7

Change LogMessageWaitStrategy to use matches()

06ba588

Change LogUtils to follow output since container start

dd6ea0d

bsideup approved these changes Apr 7, 2017

View reviewed changes

Add 'times' parameter to WaitingConsumer and LogMessageWaitStrategy

6634f8b

rnorth merged commit c2eae85 into testcontainers:master Apr 10, 2017

kiview deleted the waitForLog branch April 10, 2017 20:32

eddumelendez mentioned this pull request May 13, 2022

Add JdbcQueryWaitStrategy #5363

Closed

kiview mentioned this pull request May 13, 2022

PostgreSQL not starting correctly if image already contains data #5359

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LogMessageWaitStrategy #322

Add LogMessageWaitStrategy #322

kiview commented Apr 3, 2017

bsideup Apr 4, 2017

kiview Apr 4, 2017

bsideup Apr 4, 2017

kiview Apr 4, 2017

rnorth Apr 9, 2017

bsideup Apr 4, 2017

kiview Apr 4, 2017

rnorth Apr 6, 2017

bsideup Apr 6, 2017

kiview Apr 6, 2017

kiview Apr 7, 2017 •

edited

Loading

rnorth Apr 9, 2017

kiview Apr 9, 2017 •

edited

Loading

rnorth Apr 10, 2017

kiview commented Apr 7, 2017

bsideup commented Apr 7, 2017

kiview commented Apr 7, 2017

kiview commented Apr 8, 2017

rnorth commented Apr 9, 2017

kiview commented Apr 9, 2017

kiview commented Apr 10, 2017

rnorth commented Apr 10, 2017

kiview commented Apr 10, 2017

Add LogMessageWaitStrategy #322

Add LogMessageWaitStrategy #322

Conversation

kiview commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview commented Apr 7, 2017

bsideup commented Apr 7, 2017

kiview commented Apr 7, 2017

kiview commented Apr 8, 2017

rnorth commented Apr 9, 2017

kiview commented Apr 9, 2017

kiview commented Apr 10, 2017

rnorth commented Apr 10, 2017

kiview commented Apr 10, 2017

kiview Apr 7, 2017 •

edited

Loading

kiview Apr 9, 2017 •

edited

Loading