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

Proof Of Concept: Alternate debug #929

Closed
wants to merge 2 commits into from

Conversation

jamestalmage
Copy link
Contributor

Alternate to #874
Possibly Fixes #342

I have tested this locally, and it seems to just work.

Run with:

ava --debug=<PORT> testfile1.js testfile2.js

In a separate terminal:

node debug localhost:<PORT>

You can see the list of available commands by typing help.

Once the first test terminates, you do need to type run to reconnect the debugger between tests. I think we could request node debug add a flag to automatically reconnect (still, I don't know why you would want to debug multiple test files, and typing run a few times isn't that bad). It seems like auto-reconnect would be much easier for existing tools to implement instead of parsing stderr`.

@jamestalmage
Copy link
Contributor Author

If we think this is a good idea, we should update to handle --debug-brk as well.

No tests yet.

Also, I wonder if we couldn't implement a simple proxy layer for the debug protocol that avoided the need for existing tools to implement automatic reconnection.

@jamestalmage
Copy link
Contributor Author

Also, I wonder if we couldn't implement a simple proxy layer for the debug protocol that avoided the need for existing tools to implement automatic reconnection.

It looks like all we would have to do is proxy it and prevent the connection from the debugger from closing: https://github.com/nodejs/node/blob/master/lib/_debugger.js#L1704

@jamestalmage
Copy link
Contributor Author

This works well with node-inspector:

  1. Add some breakpoints to some tests.

  2. install and launch node-inspector.

    npm install -g node-inspector
    node-inspector
  3. Execute AVA with debug option in a different terminal

    ava --debug test/foo.js
    
  4. Open http://127.0.0.1:8080/?port=5858 in your browser

Works pretty well.

@develar
Copy link
Contributor

develar commented Jun 20, 2016

For non-IDE users it will be simpler than #874 . But IDE user relies on IDE and for IDE it ts better to use standard, tool-agnostic way :)

This PR:

  • avoid main process stop and debug. Otherwise CLI user will need to manually continue main process. Doesn't matter for IDE user.
  • automatically set concurrency to 1 to avoid debug port number collision. Doesn't matter for IDE user.

I have no relation to AVA, and I will not use this (because I am IDE user), but as happy AVA user I am glad that AVA will be suited for all, 👍

}

return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything we can request from Node.js to make this easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not Node.

But I have ideas for meow: sindresorhus/meow#44

@sindresorhus
Copy link
Member

👍 Very nice!

I tried it out and works great :)

@rhalff
Copy link

rhalff commented Jul 24, 2016

I've tried this branch with webstorm and for this to work I had to accept another command line option: --expose-debug-as=v8debug.

This is something specific to webstorm, see: nodejs/node#7102

I wonder whether the code specific to --debug and --debug-brk is needed at all, in a sense ava does not need to be aware of these parameters, while they are just passed as extra options to childProcess.fork.

Instead of debug the Api could accept a different option execArgv:

 var api = new Api({
    execArgv: ...

These options will then be passed directly to childProcess.fork.

It would be nice if these extra arguments would not have to be specifically specified, this would enable any node parameter to be passed along and let the error handling be handled by the node cli itself.

To do this, perhaps make sure all options are covered by the type list (init and no-cache are still missing): https://github.com/avajs/ava/blob/master/cli.js#L81-L107

And then use this list to filter the command line arguments and pass any unknowns using the new execArgv option.

@develar
Copy link
Contributor

develar commented Jul 25, 2016

@rhalff Please specify WebStorm version. Due to nodejs changes, WebStorm < 2016.1 doesn't support NodeJS 6+ (explicit --expose-debug-as=v8debug is required). Please use 2016.2

@rhalff
Copy link

rhalff commented Jul 25, 2016

@develar I'm using WebStorm 2016.2 (Build #WS-162.1121.31, built on July 9, 2016)

It seems webstorm 2016.2 includes --expose-debug-as=v8debug by default without having to specify it,
which doesn't make much difference with .1 where it had to be added manually.

I've created a branch here: master...rhalff:exec-argv-increase-debug-port which will increase the port number on each fork and pass any other node command line argument as is.

Webstorm is configured like this:

ava_debug

Webstorm itself will then detect the debug port for each child process:

listening

I'm not sure this is a universal fix though and whether the automatic detection of the extra debug port is something specific to webstorm only.

e.g. If I run the same command from the command line only one port is reported.

n.b. the --verbose option is needed while the spinner behaved weird within the debug console of webstorm.

@develar
Copy link
Contributor

develar commented Jul 25, 2016

@rhalff Thanks for clarification, if you use WebStorm, please use another PR #874 (comment)

This PR is not about IDE support.

@SimenB
Copy link

SimenB commented Sep 6, 2016

Can this be made to work with --inspect in newer nodes as well? No need for node-inspector then

@knownasilya
Copy link

knownasilya commented Sep 15, 2016

Would love to see this merged soon 👍

@novemberborn
Copy link
Member

To recap, this adds a --debug option to the ava CLI command. AVA then starts its child processes (for running the test files) with debugging enabled (and the debugger listening at the specified port, if any). To ensure the port is available it only runs a single test file at a time.

I think we should support --inspect now. I'm not sure how much value there is in opening the debugging port for other debuggers.

We'd need to check if the inspector URL is the same for each child process. The user would still have to reload the browser window every time a new child process is started. Ultimately perhaps we can use a proxy to inject an AVA experience into the inspector ;-)

@jamestalmage any thoughts on the above? What's your work load like? Should this be closed and punted to an issue instead?

@novemberborn
Copy link
Member

Closing due to inactivity. @jamestalmage has been incredibly busy.

If anybody wishes to take this on please comment.

@yamsellem
Copy link

Does anyone here as ever succeed to debug an ava test in Atom?

I've been using a debugger in Atom for a while, but debugging tests has been a headache since.

Thanks.

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.

None yet

8 participants