Change PostgreSQLContainer wait behaviour (fix #317)#327
Change PostgreSQLContainer wait behaviour (fix #317)#327rnorth merged 4 commits intotestcontainers:masterfrom
Conversation
|
|
||
|
|
||
| Predicate<OutputFrame> waitPredicate = outputFrame -> | ||
| outputFrame.getUtf8String().matches(regEx); |
There was a problem hiding this comment.
This is an out of the way way to mention this, but I've had this kind of code throw a NullPointerException when the regEx doesn't match any output. Pretty sure it's because getUtf8String returns null. I don't see the harm in having that return an empty string instead of null, but haven't tested it. With the code the way it is, I think we could use a null check here.
There was a problem hiding this comment.
Good point, this means it's a problem for LogMessageWaitStrategy as well I assume?
@rnorth Would it be okay for getUtf8String() to return an empty String? I'd prefer it.
There was a problem hiding this comment.
Yeah, I'd prefer having getUtf8String return an empty string too. Otherwise I think we end up with null checks in all kinds of places, like https://github.com/testcontainers/testcontainers-java/blob/master/docs/usage/options.md#waiting-for-container-output-to-contain-expected-content.
There was a problem hiding this comment.
Yes, that sounds like a good idea - thanks.
There was a problem hiding this comment.
I've had a look - I can't see how ToStringConsumer could return null though. It uses:
byte[] bytes = stringBuffer.toByteArray();
return new String(bytes, Charsets.UTF_8);If stringBuffer is empty, bytes becomes an empty byte array. new String(...) does create an empty String (not null) if passed a zero-length byte array.
Have I missed something?
There was a problem hiding this comment.
We're talking about OutputFrame::getUtf8String, not ToStringConsumer::getUtf8String...
There was a problem hiding this comment.
I've added the change in this PR.
Can someone with write access please retry the TravisCI build? The same build was green in my branch, so I think this might have been some problem with Travis or Docker in the build.
|
Using |
|
@kiview that's a good point - would be interesting if we can break the inheritance hierarchy. @asafm has kinda persuaded me to give that approach a chance :) That said, it's not that easy to put the genie back in the bottle - we'd just need to consider whether this would cause any breaking changes (e.g. people using I think it would be worth considering that as a separate change from this PR. I am, in general, meaning to kick off conversations about what the next iteration of the API will look like soon, so it's something we could consider at that stage. |
|
@rnorth I'd also like to perform this refactoring in a seperate PR. Regarding the failing codacy check: |
|
This looks good to me - no worries about the codacy warning; that's for guidance only and I agree with you on it. |
I've changed the
PostgreSQLContainerto use theWaitingConsumerfor waiting for Log output, instead of performing SQL queries. It's basically the same code as inLogWaitStrategy, but I've figured we'll refactor theJdbcDatabaseContainersat a later stage, to make use of aWaitingStrategy?This fixes #317