Change PostgreSQLContainer wait behaviour (fix #317)

[kiview]

I've changed the PostgreSQLContainer to use the WaitingConsumer for waiting for Log output, instead of performing SQL queries. It's basically the same code as in LogWaitStrategy, but I've figured we'll refactor the JdbcDatabaseContainers at a later stage, to make use of a WaitingStrategy?

This fixes #317

[dbyron0]

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.

[kiview]

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.

[dbyron0]

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.

[rnorth]

Yes, that sounds like a good idea - thanks.

[rnorth]

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?

[dbyron0]

We're talking about OutputFrame::getUtf8String, not ToStringConsumer::getUtf8String...

[rnorth]

Ah, sorry!

[kiview]

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.

[kiview]

@rnorth Since you're already performing a refactoring of the Browser containers to use WaitStrategy in this PR, should I refactor this implementation to also rely on a WaitStrategy?

[kiview]

Using LogMessageWaitStratgy works great, I wonder if PostgreSQLContainer still needs to extend JdbcDatabaseContainer now?

[rnorth]

@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 PostgresContainer and using methods that might cease to exist on its public API, like mounting volumes, etc).

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.

[kiview]

@rnorth I'd also like to perform this refactoring in a seperate PR.

Regarding the failing codacy check:
I'm personally fine with using package scoping, if package scoping is all that's needed. I can however of course change the constants back to public scoping if you prefer it that way.

[rnorth]

This looks good to me - no worries about the codacy warning; that's for guidance only and I agree with you on it.
Will merge!