Skip to content

set child process debug port to an available port - fixes #342 - #874

Merged
sindresorhus merged 4 commits into
avajs:masterfrom
develar:debug2
Aug 12, 2016
Merged

sindresorhus merged 4 commits into
avajs:masterfrom
develar:debug2

Conversation

@develar

@develar develar commented May 25, 2016 •

Copy link
Copy Markdown
Contributor

Continue #613

More robust and maintainable approach was found. Just compute required data before main promise for all files. So, it is simple to add new function in to the existing promise chain. Yeah, I spend a lot of time trying to fix last failed test and decided to rework solution.

This also doesn't handle the case where the user would do --debug-brk 213 instead of --debug-brk=213.
#613 (comment)

This syntax is not supported because node supports only --debug=, but not --debug <port number>.

develar-work:~ develar$ node --debug 12334 foo.js
Debugger listening on port 5858
module.js:442
    throw err;
    ^

Error: Cannot find module '/Users/develar/12334'

Can you add some tests?
#613 (comment)

Tests added. Initially as heavy-weight tests, but I got weird error Test files must be run with the AVA CLI if I run with nyc (passed without nyc). So, I decided to write unit tests.

@jamestalmage

Copy link
Copy Markdown
Contributor

In addition to unit tests, you should have a few integration tests. We can help figure out why NYC causes a failure.

@develar
develar force-pushed the debug2 branch 2 times, most recently from 7f63137 to 8815163 Compare May 27, 2016 09:41
@develar

develar commented May 27, 2016

Copy link
Copy Markdown
Contributor Author

you should have a few integration tests

Integration test added. Maybe on CI it will be passed, but on my machine I got "Test files must be run with the AVA CLI:" from child stderr if use nyc.

@develar

develar commented May 27, 2016

Copy link
Copy Markdown
Contributor Author

CI failed " not ok AvaError: test/fixture/debug-arg.js exited with a non-zero exit code: 1" Hmm... it works for me — I use fork of ava in my project and can debug tests in IntelliJ IDEA.

Something is wrong with nyc, but I don't see how it it is connected.

If you will have no clue, I will investigate what's wrong with nyc.

@jamestalmage

Copy link
Copy Markdown
Contributor

As I recall, in linux, ports below some number (1024?) are "privileged", and need some special permissions to run. Try bumping your tests to use something higher.

@develar

develar commented May 27, 2016

Copy link
Copy Markdown
Contributor Author

Try bumping your tests to use something higher.

Port here doesn't matter — for child we use random free port.

@jamestalmage

Copy link
Copy Markdown
Contributor

Then what does --debug=0 mean?

@jamestalmage

Copy link
Copy Markdown
Contributor

The build runs fine locally on OSX. It's either Linux or Travis specific.

Travis could be blocking this somehow.

@develar

develar commented May 27, 2016

Copy link
Copy Markdown
Contributor Author

The build runs fine locally on OSX.

I use OS X and it is failed with nyc.

Then what does --debug=0 mean?

Args of the main process. In the tests we don't start main process, we start child only (if I am not wrong). Purpose of this fix is — replace port in the main process args to another free port.

Ok, I will investigate what's wrong with nyc.

@develar

develar commented May 27, 2016

Copy link
Copy Markdown
Contributor Author

AppVeyor build succeeded only because npm run test-win — in the test-win nyc is not called.

@jamestalmage

Copy link
Copy Markdown
Contributor

Ahhh. Yes. This is almost certainly a spawnwrap bug then.

@jamestalmage

Copy link
Copy Markdown
Contributor

@develar

develar commented Jun 10, 2016

Copy link
Copy Markdown
Contributor Author

Sorry for delay, I will investigate it soon. Currently I am working to support new v8 inspector in the IntelliJ IDEA. --inspect has the same problem — no way to ask node to use any port.

@jamestalmage

Copy link
Copy Markdown
Contributor

@develar - The NYC bug is fixed. Any chance of finalizing this soon?

@develar

develar commented Jun 18, 2016 •

Copy link
Copy Markdown
Contributor Author

Rebased — tests passed.
--inspect and combination --inspect + --debug-brk supported (this combination should be used to debug tests because in case of just --inspect tests can be executed before debugger attached).

Comment thread test/api.js
test('pass ' + execArgv.join(' ') + ' to fork', function (t) {
t.plan(expectedInspectIndex === -1 ? 3 : 2);

var api = new Api({testOnlyExecArgv: execArgv});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should stub process.execArgv rather than needing a test-only option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should stub

Not quite understand, do you mean that we should replace process.execArgv during test? I think, it is not acceptable to touch global state.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you mean that we should replace process.execArgv during test?

Yes.

I think, it is not acceptable to touch global state.

True, but tests are special.

Something I've done in other projects is to have a process module which exports the global process, and then stubbing that through Proxyquire. But let's not do that unless we really have to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, but tests are special.

I think, some day AVA tests will be also executed in parallel, so, it is not wise to сhange properties in the global process object.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think, some day AVA tests will be also executed in parallel, so, it is not wise to сhange properties in the global process object.

We can deal with it then.

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.

Even when we do self test with AVA, the parallel processes will have their own process.execArgv. Test isolation FTW!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the parallel processes will have their own process.execArgv

But not tests in the same file ;)

Ok, up to you, I removed option and replace process.execArgv.

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.

But not tests in the same file ;)

True. Though you can always just do test.serial for the few that would interfere.

@novemberborn

Copy link
Copy Markdown
Member

This looks pretty good @develar!

I wonder if there's anybody who'd like to install this PR locally to debug their tests?

@jamestalmage

Copy link
Copy Markdown
Contributor

Hmm.

I kinda think we're going about this the wrong way. Instead of parsing process.execArgv, let's use process.argv:

ava --debug=<PORT>

If a user is debugging their tests, they aren't going to care about attaching a debugger to the parent process.

Using the debug flag would imply --concurrency=1, so multiple processes don't compete for the same port.

Then the debug client can connect to a single port, no parsing stderr for port numbers (which seems unreliable IMO).

IMO, If you really want to do it this way (find random open ports and print to stderr for parsing). Then that should be implemented as a separate tool using spawn-wrap (that is what nyc uses under the hood to give us coverage across many processes). That allow much wider reuse.

@develar

develar commented Jun 20, 2016 •

Copy link
Copy Markdown
Contributor Author

@jamestalmage Why? User just run debug in the IDE. Or run debug using standard node args. That's all. Nothing more. No need to reinvent the wheel and introduce new tool-specific API. User just use standard tools and way to debug.

JetBrains IDE — we reimplement multiprocess debug specially for AVA. It just works. No multiple debug tabs anymore, it is like chrome workers (you can select "thread" using combo box if need). You just hit "debug" and it works (in context runner for AVA not yet implemented, but it is another task).

Chrome using --inspect — copy url and debug. Yes, it is not seamless as in the IDE, but... IDE is IDE. I think, you told exactly about it — simplify debug using some Ava-specific tool.

But, at first, I think, we should support debug using standard tools and way.

@jamestalmage

jamestalmage commented Jun 20, 2016 •

Copy link
Copy Markdown
Contributor

we reimplement multiprocess debug specially for AVA

Is that available for us to take a look at? It sounds kinda awesome. Having multiple processes running at a time sounds cool, but I'm not sure how useful it is. Maybe you could debug two test-files that interleave disk operations in some way with clever breakpoints? IDK.

Either way, I don't think we want them to use execArgv, especially with --debug-brk. Using execArgv with --debug-brk causes it to pause the main process. So you need to manually continue it each time before child processes are launched.

node --debug node_modules/.bin/ava tests/foo.js

vs.

ava --debug test/foo.js

The second is more convenient.

See #929 - It works really well with node-inspector right now, and the CLI command node debug localhost:<PORT> works OK (you need to manually reconnect between tests by typing run). I tried this PR out locally, and I didn't find it nearly as convenient using node-inspector as #929. That might be me misunderstanding how this supposed to work (some basic instructions on how I should use this with node-inspector or similar would be good).

@develar

develar commented Jun 20, 2016 •

Copy link
Copy Markdown
Contributor Author

Is that available for us to take a look at?

Yes, these changes available since 2016.1 release. You can try 2016.2 EAP — IDEA or WebStorm. --inspect not yet supported for child process (it is in the unreleased 2016.3 (163 branch)).

Video: https://youtu.be/C75UwuZXI98

Having multiple processes running at a time sounds cool, but I'm not sure how useful it is.

I use --match if I want to debug test. I run npm run test in terminal and then debug failed tests in the IDE.

Using execArgv with --debug-brk causes it to pause the main process.
So you need to manually continue it each time before child processes are launched.

IDE uses this break to set breakpoints, find sourcemaps and then continues debug automatically.

@develar

develar commented Jun 20, 2016 •

Copy link
Copy Markdown
Contributor Author

#929 is about how to simplify AVA debug in the CLI using special ava specific CLI. This PR is to make AVA debug working using standard way (i.e. IDE vendor should not explicitly support AVA). Strictly speaking, this PR is a workaround of nodejs/node#5025

@develar

develar commented Jun 20, 2016

Copy link
Copy Markdown
Contributor Author

Maybe you could debug two test-files that interleave disk operations in some way with clever breakpoints? IDK.

You can use conditional breakpoints.

@sindresorhus sindresorhus changed the title fix #342 set child process debug port to available set child process debug port to an available port - fix #342 Aug 12, 2016
@sindresorhus sindresorhus changed the title set child process debug port to an available port - fix #342 set child process debug port to an available port - fixes #342 Aug 12, 2016
@sindresorhus
sindresorhus merged commit c268c8d into avajs:master Aug 12, 2016
@sindresorhus

Copy link
Copy Markdown
Member

Landed. Finally :)

Thanks for doing the hard work here @develar and persevering through this long review.

@develar

develar commented Aug 17, 2016

Copy link
Copy Markdown
Contributor Author

@korsmakolnikov Please specify IDE version, do you use ava from sources?

@prigara prigara mentioned this pull request Aug 17, 2016
@KeKs0r

KeKs0r commented Sep 20, 2016 •

Copy link
Copy Markdown

@develar It does not work for me. I am using this version of webstorm:

WebStorm 2016.2
Build #WS-162.1121.31, built on July 9, 2016
JRE: 1.8.0_76-release-b216 x86_64
JVM: OpenJDK 64-Bit Server VM by JetBrains s.r.o

And Ava from master, but my debugger just never stops. I was trying with single file debugging (without match):
image

Not sure why the debugger never stops. It also does not terminate.

@develar

develar commented Sep 20, 2016

Copy link
Copy Markdown
Contributor Author

@KeKs0r Please use WebStorm 2016.3 EAP

@mightyiam

Copy link
Copy Markdown
Contributor

Is this documented anywhere, please? Does it need to be, or will it simply work with anything that debugs node, like Chrome DevTools?

@develar

develar commented Nov 16, 2016

Copy link
Copy Markdown
Contributor Author

@mightyiam

Copy link
Copy Markdown
Contributor

How likely is this to ever work in Chrome DevTools?

@sindresorhus

Copy link
Copy Markdown
Member

@mightyiam It's planned with #929, but that PR stalled.

Workaround for now. Put a debugger; statement somewhere in your code and run:

$ node --inspect node_modules/ava/profile.js test.js

Where node_modules/ava/profile.js is the relative path to that file.

@develar

develar commented Nov 17, 2016 •

Copy link
Copy Markdown
Contributor Author

@mightyiam In addition to --inspect you need to pass --debug-brk So, Chrome will stop on first line.

This will output a link that you can open in Chrome. After opening that link, the Chrome Developer Tools will be displayed, and a breakpoint will be set at the first line of the Jest CLI script (this is done simply to give you time to open the developer tools and to prevent Jest from executing before you have time to do so). Click the button that looks like a "play" button in the upper right hand side of the screen to continue execution. When Jest executes the test that contains the debugger statement, execution will pause and you can examine the current scope and call stack.

Docs from Jest is true and correct for AVA as well. (Irony that currently Jest debug doesn't work due to NodeJS bug nodejs/node#7593, only AVA debug works ;)) So, you can debug in Chrome DevTools also.

@mightyiam

Copy link
Copy Markdown
Contributor

Thanks!

@bas-ie

bas-ie commented Feb 28, 2017

Copy link
Copy Markdown

Just because I couldn't find this anywhere else: when using VS Code as a debugger, you can coerce AVA to behave by using @sindresorhus 's suggestion as part of a launch.json config:

{
    "version": "0.2.0",
    "configurations": [
        {
            "type": "node2",
            "request": "launch",
            "name": "Launch Program",
            "program": "${workspaceRoot}/tests/index.test.js",
            "cwd": "${workspaceRoot}",
            "sourceMaps": true,
            "runtimeArgs": [
                "${workspaceRoot}/node_modules/ava/profile.js"
            ]
        }
    ]
}

@ImTheDeveloper

Copy link
Copy Markdown

Worth mentioning that this setup worked for me but the one bit I had missing was that I forgot to install the webstorm debug helper in chrome https://chrome.google.com/webstore/detail/jetbrains-ide-support/hmhgeddbohgjknpmjagkdomcpobmllji?hl=en-GB

@dohomi

dohomi commented Jun 13, 2017 •

Copy link
Copy Markdown
Contributor

@develar @mightyiam did you guys managed to test AVA tests in Webstorm with the current 2017.2? I use the configuration mentioned in this document: https://github.com/avajs/ava/blob/master/docs/recipes/debugging-with-webstorm.md
It seems that Webstorm is starting the debugging mode with: --inspect-brk=INT and due to this it results in:
address is already in use

@melisoner2006

Copy link
Copy Markdown
Contributor

I was also having problem with debugging ava unit tests with Webstorm 2017.2. It didn't stop at the break points. Adding inspect-brk to node options fixed the problem. Here's my SO question & answer. Thanks @develar and @mightyiam for mentioning the node options.

@novemberborn

Copy link
Copy Markdown
Member

@melisoner2006 would you be interested in opening a PR to fix the recipe?

@melisoner2006

Copy link
Copy Markdown
Contributor

@novemberborn I'd love to. I'll get on it now.

@Gregoyle

Copy link
Copy Markdown

@dohomi Did you ever get this working? I'm having a hard time getting the WS debugger to work. It could be related to the fact that our project is using babel. @melisoner2006 Was your project ES6 or something else?

@melisoner2006

melisoner2006 commented Feb 20, 2018 •

Copy link
Copy Markdown
Contributor

@Gregoyle Yes, my project was ES6.

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.