Skip to content

remove airplaine dependency for tests/test_back.js#L137#1079

Closed
gr2m wants to merge 0 commit into
masterfrom
1077/remove-airplane-setting-for-test_back-L137
Closed

remove airplaine dependency for tests/test_back.js#L137#1079
gr2m wants to merge 0 commit into
masterfrom
1077/remove-airplane-setting-for-test_back-L137

Conversation

@gr2m
Copy link
Copy Markdown
Member

@gr2m gr2m commented Feb 27, 2018

closes #1078, part of #1077

Comment thread tests/test_back.js Outdated
response.writeHead(200)
response.end()
})
server.listen(0, () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will the tests run in parallel? Because, it would be tough to figure out the behaviour of our tests in such a case if all of them use port 0 to setup the local server.

Do you think it makes sense to persist the server across tests, if they all are in the same file?

I also have some concerns with the usage of port 0 which is reserved for TCP/IP networking to do some fancy socket programming things. We can probably just use good 'ol 80(80) if we decide to persist the server?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

setting port to 0 lets the server pick an available port, so there won’t be any conflicts, see https://nodejs.org/api/net.html#net_server_listen_port_host_backlog_callback

Turns out the 0 argument can be omitted altogether, I’ll give this a try :)

@gr2m gr2m force-pushed the 1077/remove-airplane-setting-for-test_back-L137 branch from 300a340 to de5279c Compare February 28, 2018 20:36
Comment thread tests/test_back.js
response.writeHead(200)
response.end()
})
server.listen(() => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note that server.listen(() => {...}) will automatically get an available port assigned, so tests can run in parallel without conflicts, see server.listen([port][, host][, backlog][, callback])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh btw, you need the semi-colons ;)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

lol yeah we can put in proper syntax rules once we addressed the existing issues & PRs. I’ll add prettier, but not yet. For now it's wild wild west I don’t care

Comment thread tests/test_back.js Outdated
host: 'localhost',
path: '/',
port: server.address().port
}, (response) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Braces not required!

@gr2m gr2m closed this Mar 1, 2018
@gr2m gr2m force-pushed the 1077/remove-airplane-setting-for-test_back-L137 branch from c29da8e to 39d30bf Compare March 1, 2018 19:33
@gr2m
Copy link
Copy Markdown
Member Author

gr2m commented Mar 1, 2018

no idea why this was auto-closed lol

@gr2m
Copy link
Copy Markdown
Member Author

gr2m commented Mar 1, 2018

oh okay, I didn’t actually work on tests/test_back.js#L137 oops

@gr2m gr2m deleted the 1077/remove-airplane-setting-for-test_back-L137 branch March 1, 2018 19:36
@gr2m
Copy link
Copy Markdown
Member Author

gr2m commented Mar 1, 2018

see my follow up at #1083

@lock
Copy link
Copy Markdown

lock Bot commented Sep 13, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock Bot locked as resolved and limited conversation to collaborators Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants