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

Revert "avoid using same port number for test (#4147)" #4478

Merged
merged 3 commits into from
Mar 24, 2020

Conversation

bartlomieju
Copy link
Member

This reverts commit 60cee4f.

Closes #4467

CC @keroxp

@keroxp
Copy link
Contributor

keroxp commented Mar 24, 2020

It's strange to get AddrInUse even though using different ports for each test...other processes in Github Actions use one of those port sometimes?

@bartlomieju
Copy link
Member Author

@keroxp there are multiple deno processes launched when running tests so there's a high chance that two of them will run simultaneously and take first port (in both cases it will be the same port).

@nayeemrmn
Copy link
Collaborator

@bartlomieju Could we pass the first port to use in the cli for the unit test runner?

@bartlomieju
Copy link
Member Author

I don't think that's necessary, before using random ports, there were no problems with AddrInUse

@ry
Copy link
Member

ry commented Mar 24, 2020

BTW the kernel has the functionality to choose a random unused port. We should use that rather than choosing a random number and hoping that it is unused.

@@ -1,6 +1,6 @@
// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
const hostname = "0.0.0.0";
const port = +(Deno.args[0] ?? "8080");
const port = 8080;
Copy link
Contributor

Choose a reason for hiding this comment

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

@bartlomieju Would you limit port range around 4500?

const serverRoutine = async (): Promise<void> => {
const server = serve(":" + port);
const server = serve(":8124");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same above

Copy link
Member

Choose a reason for hiding this comment

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

Let's revert and do another fix up patch rather than conflate the two. It will be easier to manage the changes.

@keroxp
Copy link
Contributor

keroxp commented Mar 24, 2020

Anyway very sorry for making test more flaky...

@ry ry merged commit 30bcf6a into denoland:master Mar 24, 2020
@ry
Copy link
Member

ry commented Mar 24, 2020

@keroxp no worries - it happens!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flaky test: netTcpListenClose
4 participants