Added support for JDBC testing in ServiceTest #1179
Conversation
There was a problem hiding this comment.
@TimMoore this PR is much larger than the initially planned. I took the time to add some minor improvements in formatting and API usage.
With respect to functionality, it doesn't have more than originally agreed, ie: it adds support for JDBC in ServiceTest and it only affects test code.
I manually tested with Hello Scala (Cassandra and JDBC) and with Hello Java (Cassandra and JDBC) and with mvn and sbt. Works as expected.
We can discuss if that's something we want to include in 1.4.0 or in 1.4.1.
|
|
||
| //#setup2 | ||
| private final Setup setup2 = defaultSetup().withCluster(true); |
There was a problem hiding this comment.
Some minor improvements. Each example in its own java class to avoid setup1, setup2, etc
| * | ||
| * @return A copy of this setup. | ||
| */ | ||
| def withCassandra(): Setup = withCassandra(true) |
There was a problem hiding this comment.
Duplicated with methods with a parameterless variant. No need to be so verbose.
withCassandra(), withJdbc() and withCluster() are clear enough. We don't need to pass true each time.
On the other hand, to disable it, one has to call withCassandra(false). Which will probably never be used because it's disabled by default.
Should we consider deprecation?
| .bindings(sBind[TestServiceLocatorPort].to(testServiceLocatorPort)) | ||
| .bindings(sBind[ServiceLocator].to(classOf[TestServiceLocator])) | ||
| .bindings(sBind[TopicFactory].to(classOf[TestTopicFactory])) | ||
| .configure("play.akka.actor-system", testName) | ||
|
|
||
| val log = Logger(getClass) | ||
|
|
||
| val b3 = | ||
| val finalBuilder = |
There was a problem hiding this comment.
The following code was very cluttered and poorly formatted. Did some cosmetics rearrangement to make it more readable.
| /** | ||
| * Enriches [[GuiceApplicationBuilder]] with a `disableModules` method. | ||
| */ | ||
| private implicit class GuiceBuilderOps(val builder: GuiceApplicationBuilder) extends AnyVal { |
There was a problem hiding this comment.
Method extensions is a good option for this kind of API. We just add one more method to the already existing fluent API.
build.sbt
Outdated
| ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.javadsl.testkit.ServiceTest#Setup.withJdbc"), | ||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.javadsl.testkit.ServiceTest#Setup.withCassandra"), | ||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.javadsl.testkit.ServiceTest#Setup.jdbc"), | ||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.javadsl.testkit.ServiceTest#Setup.withJdbc") |
There was a problem hiding this comment.
Should we exclude the testkit projects from MiMa entirely?
| * | ||
| * @return A copy of this setup. | ||
| */ | ||
| def withCassandra(): Setup = withCassandra(true) |
| .bindings(sBind[TestServiceLocatorPort].to(testServiceLocatorPort)) | ||
| .bindings(sBind[ServiceLocator].to(classOf[TestServiceLocator])) | ||
| .bindings(sBind[TopicFactory].to(classOf[TestTopicFactory])) | ||
| .configure("play.akka.actor-system", testName) | ||
|
|
||
| val log = Logger(getClass) | ||
|
|
||
| val b3 = | ||
| val finalBuilder = |
| } else if (setup.jdbc) { | ||
|
|
||
| initialBuilder | ||
| .configure(CassandraTestConfig.clusterConfig()) |
There was a problem hiding this comment.
I guess we should move it somewhere other than CassandraTestConfig (doesn't have to be in this PR).
There was a problem hiding this comment.
Let's do it on this PR. Otherwise, that will be another open issue filling the backlog
There was a problem hiding this comment.
After a further look, it seems that this whole CassandraTestConfig needs a re-arrangement.
Java and Scala modules are not aligned. Java has CassandraTestConfig and TestUtil while the Scala one has two variants of TestUtil.
CassandraTestConfig (in java dsl) is also confusing, because it's not in persistence-cassandra module, but in persistence. I won't refactor it in this PR and send another one for that. Seems too intrusive and doesn't relates to this one. Is more about re-arrangement and clean-up.
|
1.4.x 22ad2e1 |
This PR adds the possibility to configure
ServiceTestto be used for JDBC test.Fixes: #304