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

Typos and inspect supports port #674

Merged
merged 6 commits into from Jan 27, 2017

Conversation

@geek
Copy link
Member

geek commented Jan 21, 2017

No description provided.

@geek geek added the feature label Jan 21, 2017
@geek geek added this to the 12.1.0 milestone Jan 21, 2017
@geek geek requested review from Marsup and cjihrig Jan 21, 2017
test/cli.js Outdated
@@ -303,6 +305,47 @@ describe('CLI', () => {
});
});

it('starts the inspector with --inspect', (done) => {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 21, 2017

Contributor

Why not just hit the /json/list endpoint?

This comment has been minimized.

Copy link
@geek

geek Jan 21, 2017

Author Member

It can do that, but there isn't a need to add the extra check, the output should be enough.

README.md Outdated

As you may know, if your tests are associated with the command `npm test`, you can already run `npm test -- --inspect` to run it with the inspector and avoid creating another command.
As you may know, if your tests are associated with the command `npm test`, you can already run `npm test -- --inspect` to run it with the inspector and avoid creating another command. If you want to listen on a specific port for the inspector, pass `--inspect={port}`. If you wish to have the inspector break on the first test script, you can pass `--inspect-brk`. Similarly, if you want to listen on a specific port with breaking on the first script use `--inspect-brk={port}`.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 21, 2017

Contributor

--inspect-brk isn't available yet is it?

This comment has been minimized.

Copy link
@geek

geek Jan 22, 2017

Author Member

updated, take a look

test/cli.js Outdated
it('starts the inspector with --inspect', (done) => {

const httpServer = new Http.Server(() => {});
httpServer.listen(0, () => {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 21, 2017

Contributor

This is trying to start the inspector on a random/available port, right? It kinda has a race condition, even though you're unlikely to run into it.

This comment has been minimized.

Copy link
@geek

geek Jan 21, 2017

Author Member

no race, since it only calls the next function when the server stops

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 21, 2017

Contributor

Yea. The race condition is with other things on the system that could grab the port.

geek added 2 commits Jan 21, 2017
README.md Outdated
@@ -25,13 +25,14 @@ global manipulation. Our goal with **lab** is to keep the execution engine as si
- `-C`, `--colors` - enables or disables color output. Defaults to console capabilities.
- `-d`, `--dry` - dry run. Skips all tests. Use with `-v` to generate a test catalog. Defaults to `false`.
- `-D`, `--debug` - print the stack during a domain error event.
- `--debug-brk` - break on the first line of application code.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 22, 2017

Contributor

Might want to specify that this is not the same as the built in --debug-brk. The built in will break on the first instruction of lab, while lab's version will break in the actual test (if I understand this correctly).

This comment has been minimized.

Copy link
@Marsup

Marsup Jan 23, 2017

Member

It's passed to node so it is the same. I don't see any code intercepting the 1st test call.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 23, 2017

Contributor

If it's the same as Node's built in, then we shouldn't document it. I'm not sure it's the behavior we'd want either.

@@ -303,6 +310,47 @@ describe('CLI', () => {
});
});

it('starts the inspector with --inspect', { skip: (NODE_MAJOR <= 4) }, (done) => {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 22, 2017

Contributor

I believe the inspector first appeared in v6.3.0. I think the skip condition should be "good enough" for CI, but could break if someone was running on v6.1.0 for example.

const childEnv = Object.assign({}, process.env);
delete childEnv.NODE_ENV;
const cli = ChildProcess.spawn(labPath, [].concat([testPath, `--inspect=${port}`]), { env: childEnv, cwd : '.' });
let combinedOutput = '';

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 22, 2017

Contributor

We don't actually need the combined output do we?

This comment has been minimized.

Copy link
@geek

geek Jan 25, 2017

Author Member

For some reason the message is sent to stderr

README.md Outdated
@@ -25,13 +25,14 @@ global manipulation. Our goal with **lab** is to keep the execution engine as si
- `-C`, `--colors` - enables or disables color output. Defaults to console capabilities.
- `-d`, `--dry` - dry run. Skips all tests. Use with `-v` to generate a test catalog. Defaults to `false`.
- `-D`, `--debug` - print the stack during a domain error event.
- `--debug-brk` - break on the first line of application code.

This comment has been minimized.

Copy link
@Marsup

Marsup Jan 23, 2017

Member

Do you really think adding this option is worth the complexity ? I never found myself debugging without this instruction, which is why I included it by default. Nor did I ever want to change port, why do that ?

This comment has been minimized.

Copy link
@geek

geek Jan 25, 2017

Author Member

This was added for testing purposes. I will try again with the debug-brk auto added, but I couldn't kill the child process since it was hung on a brk

bin/lab Outdated
'--inspect', // node's inspect
'--debug-brk' // you probably want to break 1st if you want to have a chance to hook onto the debugger before the tests run
);
else { // Node.js native inspector & debugger

This comment has been minimized.

Copy link
@Marsup

Marsup Jan 23, 2017

Member

"V8 debugger" if that's what it's called.

Copy link
Member

Marsup left a comment

Minor details, hope we're good to release soon ;)

README.md Outdated
@@ -505,13 +505,13 @@ number of incomplete assertions, their location in your code and return a failur

## Debuggers

**lab** can be started with the option `--inspect` which will run it with the Node.js native debugger enabled, breaking on the 1st instruction.
**lab** can be started with the option `--inspect` which will run it with the V8 Inspector and Node.js debugger enabled

This comment has been minimized.

Copy link
@Marsup

Marsup Jan 26, 2017

Member

Duplicate wording ?

bin/lab Outdated
'--debug-brk' // you probably want to break 1st if you want to have a chance to hook onto the debugger before the tests run
);
else { // V8 inspector
args = args.concat([inspectArg, '--debug-brk']);

This comment has been minimized.

Copy link
@Marsup

Marsup Jan 26, 2017

Member

I feel like we can go back to my const + push, can't we ?

@geek geek dismissed Marsup’s stale review Jan 27, 2017

Fixed

@geek geek merged commit c718b6c into hapijs:master Jan 27, 2017
2 checks passed
2 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek geek deleted the geek:typos branch Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.