Skip to content
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.

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

use random port in test so it won't fail on already listening #5162

Merged
merged 1 commit into from Sep 28, 2023

Conversation

rluvaton
Copy link
Contributor

In my machine, the docker desktop is using port 9999, so replacing it to 0 so it won't fail...

$ lsof -i:9999
COMMAND    PID     USER   FD   TYPE             DEVICE SIZE/OFF NODE NAME
com.docke 1752 myuser  295u  IPv6 0x83cfbdc03dd8f0bf      0t0  TCP *:distinct (LISTEN)

$ ps aux | grep 1752
myuser          1752   0.8  0.1 409650768  83072   ??  S    12:26AM   0:53.99 /Applications/Docker.app/Contents/MacOS/com.docker.backend -watchdog -native-api

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dougwilson
Copy link
Contributor

I think this is the only test that given a specific port is actually listens on that, but the test never asserts it, so probably fine to make it 0. We probably should add a test that given port x it actually listenes on port x somewhere though.

@rluvaton
Copy link
Contributor Author

Any blocker from merging this?

@dougwilson
Copy link
Contributor

Sorry @rluvaton no, I had just left a general comment, not a review change request. I will get it merged shortly, just wanted to give it at least like 24-48 hours for comments and stuff.

@rluvaton
Copy link
Contributor Author

@dougwilson I updated with the requested test, can you re-review?

@krzysdz
Copy link

krzysdz commented Apr 12, 2023

Your new tests will fail, because older Node.js versions don't support Promises.

test/app.listen.js Outdated Show resolved Hide resolved
@rluvaton
Copy link
Contributor Author

Your new tests will fail, because older Node.js versions don't support Promises.

thanks, removed the use of Promises 😢

test/app.listen.js Outdated Show resolved Hide resolved
@rluvaton
Copy link
Contributor Author

For some reason, it looked like the appveyor failed for unrelated reason
image

@rluvaton
Copy link
Contributor Author

Any thoughts or maybe a rerun so we can merge this

@rluvaton
Copy link
Contributor Author

Ping

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from f5bd4de to 98f3ef4 Compare May 16, 2023 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants