feat: support random port - #394
Conversation
📝 WalkthroughWalkthroughThe server's ChangesRandom Port Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/random-port-support.js`:
- Around line 13-15: The test currently verifies the logger side-effect by
checking that log.info received a non-zero port (via the info({port}) hook and
listeningPort), but it should assert the actual listen event payload/contract
instead; modify the test to attach to the server/emitter's 'listening' event (or
the function that emits the event) and assert the emitted payload contains the
expected port value (e.g., payload.port === expectedPort) rather than only
relying on the info hook, and update the other occurrence at the block around
lines 34-35 likewise to assert the event payload directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb6ef21d-226e-465f-9e42-0537c9b592b0
📒 Files selected for processing (2)
src/index.jstest/random-port-support.js
| info({port}) { | ||
| listeningPort = port; | ||
| }, |
There was a problem hiding this comment.
Test validates logger side-effect, not the listen-event contract.
This assertion only proves log.info received a non-zero port. It does not verify the listen event payload/contract described in the PR objective, so the feature can regress while this test still passes. Please assert the event payload directly (or explicitly align the objective/docs to “log output” instead).
Suggested adjustment
- let listeningPort = 0;
+ let listeningPort = 0;
+ let eventPort = 0;
before( (done) => {
server = new FtpServer({
log: {
info({port}) {
listeningPort = port;
},
debug() {}
},
...
});
+ // Replace 'listen' with the actual public event name if different.
+ server.once('listen', ({port}) => {
+ eventPort = port;
+ });
server.listen().then(done).catch(done);
});
it('should emmit the assigned port on listen event', () => {
expect(listeningPort).to.be.above(0);
+ expect(eventPort).to.equal(listeningPort);
});Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/random-port-support.js` around lines 13 - 15, The test currently
verifies the logger side-effect by checking that log.info received a non-zero
port (via the info({port}) hook and listeningPort), but it should assert the
actual listen event payload/contract instead; modify the test to attach to the
server/emitter's 'listening' event (or the function that emits the event) and
assert the emitted payload contains the expected port value (e.g., payload.port
=== expectedPort) rather than only relying on the info hook, and update the
other occurrence at the block around lines 34-35 likewise to assert the event
payload directly.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what event(s) FtpServer emits in src/index.js on listen
rg -n "emit" src/index.js -A2 -B2
Length of output: 1169
🏁 Script executed:
#!/bin/bash
# Check the full listen method in src/index.js
rg -n "listen" src/index.js -A5 -B2
Length of output: 939
@iamart, I need to correct my earlier suggestion — after checking the source, FtpServer.listen() does not emit a 'listen' event at all. It only calls this.log.info(...) and resolves the promise with the string 'Listening'. So server.once('listen', ...) would never fire, making my diff invalid.
To properly establish a testable public contract, the fix needs a small change in both src/index.js and the test. Two options:
Option A — Resolve the promise with the port (minimal change)
src/index.js:
- resolve('Listening');
+ resolve(listeningPort);test/random-port-support.js:
- let listeningPort = 0;
+ let listeningPort = 0;
...
- server.listen().then(done).catch(done);
+ server.listen().then((port) => {
+ listeningPort = port;
+ done();
+ }).catch(done);
...
it('should assign a port greater than 0', () => {
expect(listeningPort).to.be.above(0);
});Option B — Emit a named event (more expressive public contract)
src/index.js:
this.log.info({ protocol: ..., ip: ..., port: listeningPort }, 'Listening');
+ this.emit('listen', { port: listeningPort });
resolve('Listening');test/random-port-support.js:
- let listeningPort = 0;
+ let listeningPort = 0;
+ let eventPort = 0;
...
+ server.once('listen', ({ port }) => { eventPort = port; });
server.listen().then(done).catch(done);
...
it('should assign a port greater than 0', () => {
expect(listeningPort).to.be.above(0);
+ expect(eventPort).to.equal(listeningPort);
});Option B aligns most closely with the PR description's language ("the #listen event") and makes the contract explicit and observable by consumers of the library. Option A is simpler if no public event was ever intended.
ᓚᘏᗢ ฅ^•ﻌ•^ฅ
2 participants
You can tell node to listen on "next available port" by setting port to 0.
If I tell ftp-srv to listen on port 0, it picks next available port as expected. But there is no way to tell which port was that because the event
#listenemits the "port I setup" instead of the port the server is listening to.On this merge I make the listen event to report the right port.
Acceptance Checklist
Review Checklist
Summary by CodeRabbit
Bug Fixes
Tests