Improve getTestServerIfRunning

[tanin47]

Pull Request Checklist

• Have you read How to write the perfect pull request?
• Have you read through the contributor guidelines?
• Have you referenced any issues you're fixing using commit message keywords?
• Have you added copyright headers to new files?
• Have you checked that both Scala and Java APIs are updated?
• Have you updated the documentation for both Scala and Java sections?
• Have you added tests for any changed functionality?

Helpful things

Fixes

Fixes #12331

Purpose

Improve the interface of TestServer. Currently, if a TestServer doesn't exist, a few functions raise an exception.

Those functions should have return None (e.g. def runningHttpPort: Option[Int]) or false (e.g. def isRunning: Boolean) instead of raising an exception.

[tanin47]

Same behavior of raising an exception is kept.

[tanin47]

Return None instead of raising an exception for both functions. The return types are the same.

[tanin47]

This function wouldn't raise an exception anymore. it would just return false.

[mkurz]

I know the approach taken was my idea in #12331 (comment), however looking at the pull request now, I would say the changes in behaviour are too much.
Now I think it does make sense for runningHttp[s]Port and runningAddress to throw an exception. Let's take runningHttpPort: Right now if it returns None that means a server is running, but the http port is not set, which is absolutely fine. If it throws an exception however, it indicates to the user that the server is not running, and because this is a utility for testing, makes the test fail. I think this is what we want actually: Make the user aware that the test setup has a problem by throwing an exception. If we would also return None in case the server is not running, the user can not determine anymore why None is returned, is it because of a port not set (and server running) or because the server is not running? In worst case, a user expects a server is running but handles the None case in a way it makes the test pass where a test should actually fail.
So throwing an exception is a good thing here IMHO.

Of course, the thing that still needs fixing is to make isRunning return false instead of throwing an exception, because here it does absolutely make no sense. That fix is easy:

• TestServer.isRunning should not throw exception but return false #12409

@tanin47 Thanks for taking the time, specially for reporting the problem. I will go with #12409 because IMHO that is all that needs to be done right now. Thanks again!

[tanin47]

No worries. Thank you for improving it.