Open lihaoyi opened 1 month ago
By naming the variable that way, isn't that requiring tests to have some knowledge of the build tool?
I think $PORT
is a standard variable, e.g. Google Cloud Run expects applications to respect it. No need for it to be specific to tests either, especially not if it is controlled by Task.apply
.
Since PORT might be set, perhaps the functionality should kick in only if it isn't set.
So to summarize, the way I would do it is make my tests use $PORT
if it's defined (with some fallback), and then I can either do PORT=3000 mill myModule.test
or do mill myModule.test
and let Mill generate a value for $PORT.
As for how to assign ports, going incrementally from some starting number is not safe since something can be listening on a port somewhere in the middle. It might be good enough, of course.
Another option though is to do something like Using.resource(new ServerSocket(0))(_.getLocalPort)
. Of course that can go wrong too, if something (whether another parallel task, or some other program) happens to grab that port at just the wrong moment, but since it's random it's pretty unlikely.
Although, I'm not sure why we need Mill to do that; tests could obtain a random ephemeral port to listen on by doing something like that themselves.
Tests already need knowledge of the build tool to use MILL_TEST_RESOURCE_DIR
, so that ship has already sailed. PORT
is an option as well. I don't have a strong opinion on the name, we can bikeshed that later.
Tests can always get an ephemeral port, but it's not always easy to manage, e.g. due to needing to plumb it through multiple subprocesses, which is why my idea was to statically assign them up front and pass it down as an environment variable. But if you could make the Mill example tests assign ports dynamically, make the Cask example tests assign ports dynamically, and write up a documentation section on the best practice of how to do it, I would consider that satisfying the requirements of this bounty
From the maintainer Li Haoyi: I'm putting a 500USD bounty on this issue, payable by bank transfer on a merged PR implementing this.
Currently Mill's parallelism causes problems for test suites which need to spin up servers on localhost ports. Because different test suites may run in different tasks and subprocesses, coordinating across them to find free ports is annoying.
The Mill example test suite and the Cask example test suite just manually assigns the different web-server-related tests unique ports, but it is error-prone and should be automated
We may need to support a configurable number of unique ports (comma separated?) for test suites that need to spawn multiple services. Maybe we can annotate tasks with
Task(freePorts = n)
to mark them as needing free ports, like how we dopersistent = true
, and then Mill can assign those tasks ports starting from some base number (e.g.9000
) via the environment variableThe end result is that Mill's own
example
suite can remove all hardcoded ports and replace them with usages of theMILL_TEST_FREE_PORT
environment variable