Allow configuring of ports in ServiceTest.startServer #3168 #3169

kflorence · 2021-02-09T22:31:44Z

Pull Request Checklist

Have you read through the contributor guidelines?
Have you signed the Lightbend CLA?
Have you added copyright headers to new files?
Have you updated the documentation?
Have you added tests for any changed functionality?

Fixes

Fixes #3168

Purpose

Adds the ability to configure the ports assigned during ServiceTest.startServer

Background Context

Trying to follow the existing patterns used in Setup.

testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala

kflorence · 2021-02-09T23:08:37Z

I'm not sure what the best way to reserve a port to use in the tests on would be. I could use ServerSocket to find an available port, but between when I close that and the test server takes it it's possible another test could take it.

I see that I also need to update the JavaDSL.

octonato

Looking good.

See my comment on adding an extra configurable Cassandra port. Would the suggested alternative be a good solution for you?

testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala

octonato · 2021-02-10T09:40:20Z

I'm not sure what the best way to reserve a port to use in the tests on would be. I could use ServerSocket to find an available port, but between when I close that and the test server takes it it's possible another test could take it.

That's correct, we have done that ourselves in the Lagom build. Lot's of flaky builds we had.

But I understood that you will had a port picked by Optic and then passed to Lagom, isn't that the case?

kflorence · 2021-02-10T21:32:46Z

@octonato correct, Optic will handle port assignment for us (it will reserve an open port). Mostly I was wondering about how I can properly test these changes in the Lagom code-base -- Ideally I would have a test case that picks a known port and uses it, and then asserts that the service started up on that port.

octonato · 2021-02-11T09:34:59Z

@octonato correct, Optic will handle port assignment for us (it will reserve an open port). Mostly I was wondering about how I can properly test these changes in the Lagom code-base -- Ideally I would have a test case that picks a known port and uses it, and then asserts that the service started up on that port.

I see. Indeed that's a concern and as I said before we have hurt ourselves with exactly what you described. Reserving a free port and failing to bind it later on because someone else picked it in the meantime.

Some projects in the Akka family (Alpakka) work with a predefined list of ports. Each test has its own port to use and so they never conflict. Moving Lagom to use that same approach, will require a lot of work though.

A possible migration path would be to have an utility that pick free ports but inside a given range (I don't know if possible). Then we can start to assign fixed ports to the tests. Starting with the one here.

…m#1584

kflorence · 2021-06-28T03:25:45Z

I have started a project for managing ports across tests: https://github.com/kflorence/port-manager (not yet published to maven, but will be soon). Maybe it will be useful for the Lagom team as well.

kflorence · 2021-06-28T03:26:32Z

I'm not sure if the documentation needs to be updated, but if so I will do that next.

kflorence · 2021-06-28T17:59:05Z

It seems this change will not be binary compatible:

[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.javadsl.testkit.ServiceTest#Setup.sslPort")
[error] lagom-scaladsl-testkit: Failed binary compatibility check against com.lightbend.lagom:lagom-scaladsl-testkit_2.12:1.6.1! Found 11 potential problems (filtered 1)
[error]  * abstract method withSsl(Boolean,Int)com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup in interface com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup.withSsl")
[error]  * abstract method withPort(Int)com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup in interface com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup.withPort")
[error]  * abstract method port()Int in interface com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup.port")
[error]  * abstract method sslPort()Int in interface com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#Setup.sslPort")
[error]  * the type hierarchy of object com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl is different in current version. Missing types {scala.runtime.AbstractFunction4}
[error]    filter with: ProblemFilters.exclude[MissingTypesProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest$SetupImpl$")
[error]  * synthetic method <init>$default$1()Boolean in object com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl has a different result type in current version, where it is Int rather than Boolean
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl.<init>$default$1")
[error]  * method apply(Boolean,Boolean,Boolean,Boolean)com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl in object com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl.apply")
[error]  * synthetic method apply$default$1()Boolean in object com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl has a different result type in current version, where it is Int rather than Boolean
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl.apply$default$1")
[error]  * method copy(Boolean,Boolean,Boolean,Boolean)com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl in class com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl.copy")
[error]  * synthetic method copy$default$1()Boolean in class com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl has a different result type in current version, where it is Int rather than Boolean
[error]    filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl.copy$default$1")
[error]  * method this(Boolean,Boolean,Boolean,Boolean)Unit in class com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("com.lightbend.lagom.scaladsl.testkit.ServiceTest#SetupImpl.this")
[error] java.lang.RuntimeException: Failed binary compatibility check against com.lightbend.lagom:lagom-scaladsl-testkit_2.12:1.6.1! Found 11 potential problems (filtered 1)

I can re-arrange the values so the new ones are all at the end, but I'm not sure how I would fix the issue of having added new methods. I could put default values on the Setup trait so that defining them is completely optional. Open to other recommendations.

kflorence · 2021-07-20T18:12:23Z

@octonato sorry to bother -- any suggestions? I don't have a lot of experience with binary compatibility.

ignasi35

I'm only concerned about the possible inconsistent setups. The new arguments can introduce confusing situations: withSsl(false, 9443).

I'd rather deprecate sslEnabled and make sslPort: Option[Int] maybe.

ignasi35 · 2021-07-21T08:00:12Z

testkit/core/src/main/scala/com/lightbend/lagom/internal/testkit/TestkitSslSetup.scala

   * @return
   */
  def enabled(
      keyStoreMetadata: KeyStoreMetadata,
      trustStoreMetadata: KeyStoreMetadata,
-      clientSslContext: SSLContext
+      clientSslContext: SSLContext,
+      sslPort: Option[Int]


Suggested change

sslPort: Option[Int]

sslPort: Option[Int] = Some(0)

Adding the default value makes the change backwards compatible. You can also duplicate the methods and add a deprecation notice on the overload without sslPort: Option[Int].

Also, if this method requires an sslPort, could the type of sslPort be Int and then add a require(sslPort > 1024 && sslPort < 65536) on the implementation.

hm, those requirements seem like they would also apply to the non-ssl port, although I was thinking that if someone is specifying a particular port we perhaps shouldn't care what port they chose. it does seem good to check if the port != sslPort though.

hm, those requirements seem like they would also apply to the non-ssl port

I'm considering the non-ssl port to be compulsory: "test MUST open a cleartext port and MAY open a TLS port".

@ignasi35 i mean require(sslPort > 1024 && sslPort < 65536) -- doesn't that also apply to the non-SSL port?

Oh, absolutely it does! I missed your point there.

ignasi35 · 2021-07-21T08:02:49Z

testkit/javadsl/src/main/scala/com/lightbend/lagom/javadsl/testkit/ServiceTest.scala

+    override def withSsl(enabled: Boolean): Setup = withSsl(enabled, port = 0)
+    override def withSsl(enabled: Boolean, port: Int): Setup = {
+      copy(ssl = enabled, sslPort = port)
+    }


Suggested change

override def withSsl(enabled: Boolean): Setup = withSsl(enabled, port = 0)

override def withSsl(enabled: Boolean, port: Int): Setup = {

copy(ssl = enabled, sslPort = port)

}

override def withSsl(enabled: Boolean): Setup = withSsl(enabled, port = 0)

override def withSsl(sslPort: Int): Setup = {

require(sslPort> 1024 && sslPort < 65536)

require(sslPort != port)

copy(ssl = true, sslPort = port)

}

ignasi35 · 2021-07-21T08:08:39Z

testkit/javadsl/src/main/scala/com/lightbend/lagom/javadsl/testkit/ServiceTest.scala

+    /**
+     * The server HTTPS port.
+     */
+    def sslPort: Int


Suggested change

def sslPort: Int

def sslPort: Option[Int]

Maybe make this an option? Then def ssl: Boolean becomes def ssl: Boolean = sslPort.isDefined?

ignasi35 · 2021-07-21T08:13:40Z

@octonato sorry to bother -- any suggestions? I don't have a lot of experience with binary compatibility.

while all the changes remain on testkit API breaking compatibility is not a big issue. Users may need to do a bit of maintenance on their code.

The error is already listing all the ProblemFilter commands you would need to list on the build for mima to ignore. Look for ProblemFilter on build.sbt or other build files and add yours. Having said that, sometimes spending a couple of minutes reducing the list (by keeping old methods and deprecating them) can help end users.

kflorence · 2021-07-21T21:13:24Z

@ignasi35 thanks for the comments, I will see if I can make some more progress this weekend.

kflorence commented Feb 9, 2021

View reviewed changes

testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala Show resolved Hide resolved

octonato reviewed Feb 10, 2021

View reviewed changes

testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala Show resolved Hide resolved

testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala Outdated Show resolved Hide resolved

kflorence added 2 commits June 26, 2021 11:51

Allow configuring of ports in ServiceTest.startServer lagom#3168 lago…

2572d0c

…m#1584

Remove cassandra port. Update javadsl.

d5a1670

kflorence force-pushed the 3168 branch from af17d83 to d5a1670 Compare June 28, 2021 03:22

kflorence changed the title ~~Allow configuring of ports in ServiceTest.startServer #3168 #1584~~ Allow configuring of ports in ServiceTest.startServer #3168 Jun 28, 2021

Fix some missing default arguments in this() constructor

3fa6292

ignasi35 reviewed Jul 21, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring of ports in ServiceTest.startServer #3168 #3169

Allow configuring of ports in ServiceTest.startServer #3168 #3169

kflorence commented Feb 9, 2021 •

edited

kflorence commented Feb 9, 2021 •

edited

octonato left a comment

octonato commented Feb 10, 2021

kflorence commented Feb 10, 2021

octonato commented Feb 11, 2021

kflorence commented Jun 28, 2021

kflorence commented Jun 28, 2021

kflorence commented Jun 28, 2021

kflorence commented Jul 20, 2021

ignasi35 left a comment

ignasi35 Jul 21, 2021

kflorence Jul 21, 2021

ignasi35 Jul 22, 2021

kflorence Jul 22, 2021

ignasi35 Jul 23, 2021

ignasi35 Jul 21, 2021

ignasi35 Jul 21, 2021

ignasi35 commented Jul 21, 2021

kflorence commented Jul 21, 2021

Allow configuring of ports in ServiceTest.startServer #3168 #3169

Are you sure you want to change the base?

Allow configuring of ports in ServiceTest.startServer #3168 #3169

Conversation

kflorence commented Feb 9, 2021 • edited

Pull Request Checklist

Fixes

Purpose

Background Context

kflorence commented Feb 9, 2021 • edited

octonato left a comment

Choose a reason for hiding this comment

octonato commented Feb 10, 2021

kflorence commented Feb 10, 2021

octonato commented Feb 11, 2021

kflorence commented Jun 28, 2021

kflorence commented Jun 28, 2021

kflorence commented Jun 28, 2021

kflorence commented Jul 20, 2021

ignasi35 left a comment

Choose a reason for hiding this comment

ignasi35 Jul 21, 2021

Choose a reason for hiding this comment

kflorence Jul 21, 2021

Choose a reason for hiding this comment

ignasi35 Jul 22, 2021

Choose a reason for hiding this comment

kflorence Jul 22, 2021

Choose a reason for hiding this comment

ignasi35 Jul 23, 2021

Choose a reason for hiding this comment

ignasi35 Jul 21, 2021

Choose a reason for hiding this comment

ignasi35 Jul 21, 2021

Choose a reason for hiding this comment

ignasi35 commented Jul 21, 2021

kflorence commented Jul 21, 2021

kflorence commented Feb 9, 2021 •

edited

kflorence commented Feb 9, 2021 •

edited