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.
|
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. |
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.