Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Use node-portfinder to avoid error when port is used #340

Merged
merged 2 commits into from
Apr 9, 2016
Merged

Conversation

thangngoc89
Copy link
Contributor

Close #320

if (err) {
log(err)

return
Copy link
Owner

Choose a reason for hiding this comment

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

why not throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was there before. You like throw. I'll do it.

@MoOx
Copy link
Owner

MoOx commented Mar 22, 2016

May be we should add integration test for this. Cause coverage is decreasing.

@thangngoc89
Copy link
Contributor Author

May be we should add integration test for this.

Sure. On it

@thangngoc89
Copy link
Contributor Author

Cause coverage is decreasing.

How do you know about that ?

@MoOx
Copy link
Owner

MoOx commented Mar 22, 2016

How do you know about that ?

You add code that is not tested :)

@@ -38,10 +38,38 @@ test.cb("should NOT throw if a CLI flag is recognized", (t) => {
)

// ...or be ok quickly
// (so we assume it's ok and kill the process, we don't need the actual build)
// (so we assume it"s ok and kill the process, we don"t need the actual build)
Copy link
Owner

Choose a reason for hiding this comment

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

‘ (alt + ')

@thangngoc89
Copy link
Contributor Author

@MoOx integration-tests was not run

const timeout = setTimeout(() => {
t.pass()
t.end()
child.kill()
}, 500)
})

test.cb("should NOT throw if port is used", (t) => {
Copy link
Owner

Choose a reason for hiding this comment

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

should, not "should NOT" ?

Copy link
Owner

Choose a reason for hiding this comment

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

hum ok nvm.

@thangngoc89
Copy link
Contributor Author

@MoOx can you have a look at the integration-tests ? original tests in that file aren't killed.

@@ -7,7 +7,7 @@ import { exec } from "child_process"
const asyncExec = asyncify(exec)

const target = join(__dirname, "..", "test-boilerplate")
const execOpts = { cwd: target }
const execOpts = { cwd: target, stdio: "inherit" }
Copy link
Owner

Choose a reason for hiding this comment

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

please don't do that. This can pollute test output.

@thangngoc89
Copy link
Contributor Author

Blocked by #346

@MoOx MoOx force-pushed the feat/portfinder branch 2 times, most recently from 7eac5a8 to 3e033ba Compare April 5, 2016 04:27
@MoOx
Copy link
Owner

MoOx commented Apr 5, 2016

@thangngoc89
Copy link
Contributor Author

No idea. I'll take a look

@thangngoc89
Copy link
Contributor Author

All tests passed on local

@MoOx
Copy link
Owner

MoOx commented Apr 5, 2016

Same here, local tests ok.

@thangngoc89
Copy link
Contributor Author

I can't print the log even with inherit

@MoOx
Copy link
Owner

MoOx commented Apr 9, 2016

woot. 5432 seems to be used by travis, I tried with 8081, and tests are ok on travis.

@MoOx
Copy link
Owner

MoOx commented Apr 9, 2016

I cleaned this PR, waiting for tests.

@MoOx
Copy link
Owner

MoOx commented Apr 9, 2016

FYI, that may be the reason :)
screen shot 2016-04-09 at 14 47 39

@MoOx MoOx merged commit f960c3d into next Apr 9, 2016
@MoOx MoOx deleted the feat/portfinder branch April 9, 2016 12:52
@thangngoc89
Copy link
Contributor Author

Haha. Nice.

@MoOx MoOx mentioned this pull request Jan 12, 2017
Merged
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