-
Notifications
You must be signed in to change notification settings - Fork 554
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
Refactor port logic in healthcheck and tests Suite #14736
Conversation
protected def fetchResults(testPort: Int, healthChecks: SingleHealthCheck*): Future[Seq[HealthCheckResult]] = { | ||
val defaultPort = 9000 | ||
val port = { | ||
Play.current.mode match { |
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.
👋
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.
🎉
51c730b
to
4004713
Compare
@@ -14,7 +15,9 @@ import org.scalatest.concurrent.ScalaFutures | |||
with WithTestWsClient { | |||
|
|||
"CDN health check" should "mimic the instance health check" in { | |||
val controller = new HealthCheck(wsClient) | |||
val testPort: Int = 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.
This is just for clarity or is it needed for the override to compile?
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.
Necessary, otherwise the port
val in the constructor closure shadows the test 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.
This is great! 👍
What does this change?
My original goal was to get rid of the
Play.current
reference into theHealthCheck
code.However I thought it would be a good time to refactor this weird logic where a port is specify to the Healthcheck, which is then used to override the OneServerPerSuite port so multiple suite don't overlap on the same port and the healthcheck is aware of this port on which it needs to run its internal check.
So now:
Suite
What is the value of this and can you measure success?
Request for comment
@alexduf @jfsoul