-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow configuring of ports in ServiceTest.startServer #3168 #3169
base: master
Are you sure you want to change the base?
Conversation
testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
testkit/scaladsl/src/main/scala/com/lightbend/lagom/scaladsl/testkit/ServiceTest.scala
Outdated
Show resolved
Hide resolved
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? |
@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. |
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. |
I'm not sure if the documentation needs to be updated, but if so I will do that next. |
It seems this change will not be binary compatible:
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 |
@octonato sorry to bother -- any suggestions? I don't have a lot of experience with binary compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @return | ||
*/ | ||
def enabled( | ||
keyStoreMetadata: KeyStoreMetadata, | ||
trustStoreMetadata: KeyStoreMetadata, | ||
clientSslContext: SSLContext | ||
clientSslContext: SSLContext, | ||
sslPort: Option[Int] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ignasi35 i mean require(sslPort > 1024 && sslPort < 65536)
-- doesn't that also apply to the non-SSL port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, absolutely it does! I missed your point there.
override def withSsl(enabled: Boolean): Setup = withSsl(enabled, port = 0) | ||
override def withSsl(enabled: Boolean, port: Int): Setup = { | ||
copy(ssl = enabled, sslPort = port) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
} |
/** | ||
* The server HTTPS port. | ||
*/ | ||
def sslPort: Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def sslPort: Int | |
def sslPort: Option[Int] |
Maybe make this an option? Then def ssl: Boolean
becomes def ssl: Boolean = sslPort.isDefined
?
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 |
@ignasi35 thanks for the comments, I will see if I can make some more progress this weekend. |
Pull Request Checklist
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
.