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?
There was a problem hiding this comment.
Fair. I'd propose test-tools 2.x as the one that keeps supporting LTS, so we could merge this to release/test-tools/2.0 instead of main, release 2.1.0, and have that be the (hopefully) final piece that fixes the LTS build. I don't expect that we'll need many changes to test-tools going forward for the sake of LTS, so I'd be ok doing all of it as patches/minors on top of that release branch.
There was a problem hiding this comment.
I was implicitly thinking of adding this change to the test-tools 2.x release branch anyway in order to consume it from LTS, just hadn't thought that it indeed doesn't make much sense in main.
There was a problem hiding this comment.
All that sounds good to me--I've opened #23429 for the change directly into 2.x (after addressing other bit of PR feedback) and will close this one.
|
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. |
There was a problem hiding this comment.
Functional changes look good to me. I think the only remaining question is if we even want to merge this to main, or directly to release/test-tools/2.0, since it's only relevant there (and there's the open PR I mentioned in one of the comments, making test-tools a private package that wouldn't be published going forward).
## 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 and #23361 for more context. --------- Co-authored-by: Abram Sanderson <[email protected]>
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.