Skip to content

Update error message: output port at which port finder failed #178

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Update error message: output port at which port finder failed #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/portfinder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ internals.getPort = function (options, callback) {
return callback(null, openPorts[0]);
}
else {
const msg = 'No open ports found in between '+ options.startPort + ' and ' + options.stopPort;
const msg = `Searching for an open port failed at port ${openPorts[0]}.`;
Copy link
Contributor

@MasterOdin MasterOdin Mar 14, 2025

Choose a reason for hiding this comment

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

I think this makes the error message worse for the single port case as it's less informative, where it sort of sounds like the library only tested port 32773 and that failed, so here's an error, where it had checked several ports.

For the multiple port case, I think it's fine for the callback variant as you get a partial array of ports it could find, whereas for the promise variant I think it's worse as similar to the single port variant, you cannot tell that it had tested other ports before failing.

I think ideally the error message would be as it is for single port and then like "Could not find 3 open ports between ...." as then it still gives the range info.

Copy link
Member Author

@eriktrom eriktrom Mar 15, 2025

Choose a reason for hiding this comment

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

Yeah I agree. Whether we wanted to change the error message and how is basically what I wanted to get your feedback on.

Thanks again for helping out. It’s a lot more likely to change and get better with two heads.

return callback(Error(msg));
}
} else {
Expand Down
6 changes: 3 additions & 3 deletions test/port-finder-multiple.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ describe('with 5 existing servers', function () {
})
.catch(function (err) {
expect(err).not.toBeNull();
expect(err.message).toEqual('No open ports found in between 32768 and 32774');
expect(err.message).toEqual('Searching for an open port failed at port 32775.');
done();
});
} else {
method(3, { stopPort: 32774 }, function (err, ports) {
expect(err).not.toBeNull();
expect(err.message).toEqual('No open ports found in between 32768 and 32774');
expect(ports).toEqual([32773, 32774, undefined]);
expect(err.message).toEqual('Searching for an open port failed at port 32775.');
expect(ports).toEqual([32773, 32774, undefined]); // Failed at port 32775
done();
});
}
Expand Down
4 changes: 2 additions & 2 deletions test/port-finder.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,13 @@ describe('with 5 existing servers', function () {
})
.catch(function (err) {
expect(err).not.toBeNull();
expect(err.message).toEqual('No open ports found in between 32768 and 32772');
expect(err.message).toEqual('Searching for an open port failed at port 32773.');
done();
});
} else {
method({ stopPort: 32772 }, function (err, port) {
expect(err).not.toBeNull();
expect(err.message).toEqual('No open ports found in between 32768 and 32772');
expect(err.message).toEqual('Searching for an open port failed at port 32773.');
expect(port).toBeUndefined();
done();
});
Expand Down