Skip to content
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

Provide a MILL_TEST_FREE_PORT environment variable for tests to use (500USD Bounty) #3802

Open
lihaoyi opened this issue Oct 22, 2024 · 2 comments
Labels

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Oct 22, 2024


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 do persistent = true, and then Mill can assign those tasks ports starting from some base number (e.g. 9000) via the environment variable

The end result is that Mill's own example suite can remove all hardcoded ports and replace them with usages of the MILL_TEST_FREE_PORT environment variable

@lihaoyi lihaoyi changed the title Provide a MILL_TEST_FREE_PORT environment variable for tests to use Provide a MILL_TEST_FREE_PORT environment variable for tests to use (500USD Bounty) Oct 24, 2024
@lihaoyi lihaoyi added the bounty label Oct 24, 2024
@nafg
Copy link
Contributor

nafg commented Oct 27, 2024

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.

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 28, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants