MockServerContainer: remove dependency to java api #833
Conversation
originally that was not good idea to depend on something which can brake binary compatibility :) this change will allow to start any mockserver container version, doesn't matter which version of the client in the classpath
lanwen
changed the title
There was a problem hiding this comment.
Actually... I'm not sure that we should make this change. Here's my thinking:
-
There is a benefit in having a binary dependency of a fixed version, because the docker image is (by default) on that same version. This makes it easier to be confident about compatibility. If a different version is needed, you can specify this in the constructor and (mostly) just as easily override the dependency version.
-
The class adds value because it provides easy access to a wired-up
MockServerClientinstance. Without that getter, its reason for existence is less (you could just provide a snippet showing how to useGenericContainerto do the same thing). This module 'earns its place' by doing (just) a bit more than such a code snippet.
If we wanted to not have a binary dependency, but still benefit from (1), then it would be awesome to do automatic JAR/docker-image dependency matching like we do for Selenium. However, I assume there isn't as much need for strict compatibility, so this is probably only a nice-to-have.
I won't close right away, but I think that's the way I'm leaning. Does my logic make sense to you?
... and now I find out from @bsideup that there is a breaking change in the mockserver API which messes this up 😆 Would it be practical to detect the JAR version (e.g. via MANIFEST.MF) and automatically select the right docker image version? |
|
Actually the REST api is pretty stable, what breaks is calling java api when initializing client. If you pick other than fixed version with broken binary compatibility, you just receive NoClassDefFound on container start. Why its useful still to have this snippet without client - is that client initialization takes 3 lines in your code, when whole class is 10 times bigger. What we can try is checking that known java version is in classpath and create a client, if no - just be noop and return null for client. |
| @@ -20,7 +21,7 @@ | |||
|
|
|||
| @Test | |||
| public void testBasicScenario() throws Exception { | |||
| mockServer.getClient() | |||
| new MockServerClient(mockServer.getContainerIpAddress(), mockServer.getMappedPort(MockServerContainer.PORT)) | |||
There was a problem hiding this comment.
would be nice to add mockServer.getServerPort() to hide the constant
bsideup
added
type/breaking-api-change
bsideup
changed the title
|
@rnorth so, are you OK with this change? |
|
Removing the client looks good to me, isn't it the same we are doing in case of Selenium? |
|
@kiview Selenium has a stable public API, this is why we never had such issue with it, but MockServer project is not strict enough about the public API and broke it in one of their latest releases |
|
Yep go for it. Some day maybe I’d like to extract the Selenium module’s automatic JAR-image pairing. But that can wait. |
originally that was not good idea to depend on something which
can brake binary compatibility :)
this change will allow to start any mockserver container version,
doesn't matter which version of the client in the classpath