Skip to content

Added MILL_TEST_FREE_PORT #4931

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Added MILL_TEST_FREE_PORT #4931

wants to merge 11 commits into from

Conversation

albassort
Copy link

Made in accordance to #3802

@albassort albassort marked this pull request as ready for review April 13, 2025 21:22
@albassort
Copy link
Author

RE: There is some failing tests on HEAD right now. I think

Run Tests / windows (17.0.14, 'integration.bootstrap[no-java-bootstrap].native.server') / run (pull_request)Failing after 9m

Might be related to the PR but im not sure. I will be investigating,

Comment on lines 55 to 58
/**
* Used to store free ports for Mill's tests to use.
*/
public static final String MILL_TEST_FREE_PORT = "MILL_TEST_FREE_PORT";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a single port or multiple. The comment is a bit vague. If it is multi, that the variable should be MILL_TEST_FREE_PORTS and we should document here, which separator is used.

var i = 0
var ports = Set.empty[Int]
for (_ <- 1 to 100) {
if (i == 6) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number 6! A parameter or a named constant might be better.

}

streams.err.println(
"Failed to generate enough ports for servers to use. Either there isn't 6 free ports on your computer, or there is a firewall issue. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number 6.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number 6.

Fixed but, maybe we don't need to do this at all.

@lihaoyi
Copy link
Member

lihaoyi commented Apr 18, 2025

This doesn't satisfy the original issue

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

@albassort
Copy link
Author

albassort commented Apr 18, 2025

This doesn't satisfy the original issue

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

I misunderstood, I thought that were to be done in a second PR. I will work on this.

@albassort albassort marked this pull request as draft April 18, 2025 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants