feat(test-tools): Support lerna in assign-test-ports #23361
Conversation
There was a problem hiding this comment.
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (1)
- tools/test-tools/package.json: Language not supported
| @@ -1,5 +1,10 @@ | |||
| #!/usr/bin/env node | |||
| const mod = require("../dist/assignTestPorts.js"); | |||
| const nconf = require("nconf"); | |||
There was a problem hiding this comment.
I was going to keep test-tools dependency-less, but figured it's not much overhead to add nconf and we use it elsewhere in our test infra (e2e test scripts). Open to writing some minimal parsing logic ourselves if reviewer would prefer that though; we don't need a particularly reusable or ergonomic thing here.
There was a problem hiding this comment.
When do dependencies for assign-test-ports get installed? Is there an existing use case where the dependencies are not installed?
I think using helper is good, but not confident this won't breaking existing workflows. :/
There was a problem hiding this comment.
All of our usage of test-tools should just come via normal dep declaration in package.json, so transitive dependencies should be fine. e.g. see changes in https://github.com/microsoft/FluidFramework/pull/23285/files#diff-483384132d69f74744804315cbd004d361f840a7a007d9cfaca2714c69428493
There was a problem hiding this comment.
+1, I think this won't be an issue. A bit related, I have another PR open to make test-tools a @fluid-private package in the client release group, so all its uses would be limited to it being a dev dependency during the build.
There was a problem hiding this comment.
So, if test-tools is becoming private, then I don't think this makes sense to merge to main. Something needs to support the LTS branch. Can either the privatization also happen in LTS OR is there a test-tools release and development branch that this should accumulate to?
|
Still working on testing this change in LTS locally. |
|
Update: my LTS branch doesn't repro the same port assignment error that originally motivated this change, but I have locally verified that:
So I'm reasonably confident this change should be ok. |
Expected, the issue only happens in the build agents where something is listening on port 8084. |
|
|
||
| // We used to hardcode port 8081 as the initial one | ||
| // but as of 2024-11-25 port 8084 is used by something in the build agent image. | ||
| // If a package that has jest tests ends up assigned that port, | ||
| // jest will fail to start. | ||
| // So try to use a port range where nothing will be listening. | ||
| const initialPortString = firstArg ?? "9000"; | ||
| // Note '_' is used for positional arguments in nconf. | ||
| const initialPort = nconf.get("_")[0] ?? 9000; |
There was a problem hiding this comment.
I'd expect nconf.get() to return a string. I think that gets handled correctly all the way because the first thing we end up doing with it is <value>++, but I think it'd be better to explicitly convert to number here.
Description
Adjusts test-tools's assign-test-ports script to support using lerna over pnpm. This allows us to use a modern test-tools version in our LTS branch. See discussion on #23285.