feature/add-first-class-debugging-support-for-tests by cdaringe · Pull Request #1360 · react/create-react-app

cdaringe · 2017-01-07T01:11:26Z

problem statement

react-scripts does not offer ready-to-roll jest debugging.

solution

add it!
- i selected a natural model where --debug-brk and debug are working flags passed to npm test. that is npm test -- --debug-brk works. i think this is a nice entry point into the system.
- auto-handle other flags that need to be massaged to make debugging work. that is, disable watch and enable runInBand

closes#594

facebook-github-bot · 2017-01-07T01:11:31Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

cdaringe · 2017-01-07T01:16:56Z

note, running node --debug-brk ./node_modules/.bin/react-scripts test --debug-brk will biff on conflicting port access to 5858, if this wasn't clear already. I did not handle this case, but it may be worth discussion?

cdaringe · 2017-01-07T01:18:19Z

🎉 greet the debugging user with some 🌶 🔥 tips

facebook-github-bot · 2017-01-07T01:19:00Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

cdaringe · 2017-01-07T01:21:29Z

any of the node debug flags are not valid input to jest. something counter convention i did in this PR was allow a debug flag (like --debug-brk) to enter via argv, where previously all of the other args were jest specific. IMHO, this is OK. the filtering may be cleaner towards the top-of-the-file

cdaringe · 2017-01-07T02:40:11Z

i'm a little puzzled w/ the CI error. it fails on being unable to locate a file, but only on some node versions. maybe a caching thing? can someone weigh in or re- kick it off?

tuchk4 · 2017-01-07T03:54:34Z

@cdaringe CI fails becasue this require at packages/react-scripts/scripts/test.js

// packages/react-scripts/scripts/test.js

utils dir is not available after eject

EnoahNetzach · 2017-01-07T14:19:22Z

Put this IDE ignore in a global .gitignore (@see#958)

EnoahNetzach · 2017-01-07T14:24:52Z

For this to work after the eject, you'll probably have to add the right path here.

cdaringe · 2017-01-07T21:19:40Z

thanks @tuchk4, @EnoahNetzach. i dropped the editor ignore commit, rebased w/ patches to restore the build. should be g2g now, post review :)

cdaringe · 2017-01-09T21:48:13Z

FYI, to reviewers, this also only accepts one flag as written. that is --debug-brk --inspect won't work, but we can do a follow up to add that support later. i'd love for someone to test it out on their box too to validate!

gaearon · 2017-02-12T02:06:03Z

Tagging with a milestone because we need to review this.

cdaringe · 2017-02-12T02:44:54Z

i'm not entirely in love with the implementation, especially now that --inspect is essentially mainstream. feel free to be harsh

Gregoor · 2017-03-23T11:08:08Z

Would love to see this land or help in making it happen. Especially as Webstorm/IntelliJ integrated testing would then work with create-react-app (related issue).

Timer · 2017-03-23T12:33:28Z

I won't have time to review this until first week of April.

xref: https://youtrack.jetbrains.com/issue/WEB-26048

lifeiscontent · 2017-04-10T03:22:24Z

@Timer any updates on this?

Timer · 2017-04-10T03:47:10Z

Just added it to my list of soonish TODOs, @lifeiscontent. Thanks for the reminder. ❤️

lifeiscontent · 2017-04-10T03:48:14Z

@Timer you rock! 👍

Timer · 2017-04-10T16:08:26Z

Considering that the legacy protocol is dead, I'm not sure if we should implement it (--debug varieties).
See Node docs and Microsoft docs.
Microsoft hints that as of Node 8.x, it (inspect) will be the official and recommended way to debug.

On that note, I'd like to see support for strictly --inspect ^ --inspect-brk.
This isn't the most optimal situation since it's an experimental protocol, however, the community seems to be rallying around it.
VSCode supports it first class, WebStorm uses it, and community packages are deprecating themselves.

Also, we should make sure it supports the port syntax --inspect=4321.
Additionally, I'd like to not extract logic to other files yet as it adds unnecessary review complexity (e.g. babelJestDefault tab, babelJestDebug tab).

@cdaringe do you happen to have any free time to bring this PR up to date?
If not, that's fine and we'll find time to take care of it.

cdaringe · 2017-04-10T21:22:59Z

On that note, I'd like to see support for strictly --inspect ^ --inspect-brk.

totally. in agreement.

Also, we should make sure it supports the port syntax --inspect=4321.

yep. agreed.

Additionally, I'd like to not extract logic to other files yet as it adds unnecessary review complexity (e.g. babelJestDefault tab, babelJestDebug tab).

if you can find a way around it, that'd be great!

i'm swamped for a few weeks--i wont be getting to it anytime soon. :(

quantuminformation · 2017-04-12T11:28:07Z

So looking fwd to this, it will also enable Jetbrains to debug JSX:

https://youtrack.jetbrains.com/issue/WEB-26048

Timer · 2017-04-12T17:12:20Z

@cdaringe not a problem, thanks so much for letting me know!

cdaringe · 2017-04-12T17:25:03Z

- babelrc: false
-});
+const babelJestDebugConfig = require('./babelJestDebugConfig');
+module.exports = babelJest.createTransformer(babelJestDebugConfig);


this was probably supposed to be Default v Debug?

cdaringe · 2017-04-12T17:26:56Z

-
+ const babelTransform = isEjecting
+ ? '<rootDir>/node_modules/babel-jest'
+ : resolve('config/jest/babelTransform' + (isDebug ? 'Debug' : '') + '.js')


@Timer, 👀, this is where/why there were two babelTransform files. we are passing the babel config as a file path, therefore, to enable sourceMaps, i added a different file. if there's another way to get those babel settings passed to jest, we could pursue that. we could also set something in the ENV to drop it down to one transform file?

Ok I hadn't reviewed deep enough to have this understanding.
Thanks for explaining!
I'll snoop around and see what our best option(s) are. 😄

thg303 · 2017-04-20T06:58:03Z

would somebody pleeaaase merge this! I am pulling my hairs on debugging for a week now :'(

quantuminformation · 2017-04-20T07:29:51Z

@thg303 you should have some hairs left before webstorms 2017.2 release

Timer · 2017-04-20T12:28:26Z

Sorry for the delay @thg303.
It's happening soon, I promise!

cdaringe · 2017-04-28T08:47:58Z

moving to #2041

avantgardnerio · 2017-08-07T09:34:48Z

The solution does not work for me, I can't figure out how to debug jest.

cdaringe · 2017-08-07T17:57:17Z

It's super old on the react-scripts time scale, I wouldn't expect it to

avantgardnerio · 2017-08-08T01:43:39Z

For those who are also stuck, it looks like you can downgrade to node 7.10 to debug:

nodejs/node#7593

cdaringe commented Jan 7, 2017
View reviewed changes

Comment threadpackages/react-scripts/scripts/test.js Outdated
cdaringeJan 7, 2017
Copy link
Copy Markdown
Author
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 greet the debugging user with some 🌶 🔥 tips

facebook-github-bot added the CLA Signed label Jan 7, 2017

cdaringe commented Jan 7, 2017
View reviewed changes

EnoahNetzach suggested changes Jan 7, 2017
View reviewed changes

🎨 add first class debugging support
14d53fb

cdaringe force-pushed the feature/improved-jest-debugging branch from c976b56 to 14d53fbCompare January 7, 2017 20:49

cdaringe changed the title ~~Feature/improved jest debugging~~feature/add-first-class-debugging-supportJan 7, 2017

cdaringe changed the title ~~feature/add-first-class-debugging-support~~feature/add-first-class-debugging-support-for-testsJan 7, 2017

EnoahNetzach approved these changes Jan 8, 2017
View reviewed changes

cdaringe mentioned this pull request Jan 10, 2017
source maps interpretation inconsistent microsoft/vscode#18373
Closed

gaearon added this to the 0.10.0 milestone Feb 12, 2017

morsh mentioned this pull request Mar 20, 2017
Debugging Tests wmonk/create-react-app-typescript#29
Closed

cdaringe commented Apr 12, 2017
View reviewed changes

cdaringe mentioned this pull request Apr 28, 2017
🎨 add first class debugging support #2041
Closed

cdaringe closed this Apr 28, 2017

cdaringe deleted the feature/improved-jest-debugging branch April 28, 2017 08:48

lockBot locked and limited conversation to collaborators Jan 21, 2019

Uh oh!

Uh oh!

Conversation

cdaringe commented Jan 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

problem statement

solution

Uh oh!

facebook-github-bot commented Jan 7, 2017

Uh oh!

cdaringeJan 7, 2017

Choose a reason for hiding this comment

Uh oh!

cdaringeJan 7, 2017

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 7, 2017

Uh oh!

cdaringeJan 7, 2017

Choose a reason for hiding this comment

Uh oh!

cdaringe commented Jan 7, 2017

Uh oh!

tuchk4 commented Jan 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EnoahNetzachJan 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EnoahNetzachJan 7, 2017

Choose a reason for hiding this comment

Uh oh!

cdaringe commented Jan 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdaringe commented Jan 9, 2017

Uh oh!

gaearon commented Feb 12, 2017

Uh oh!

cdaringe commented Feb 12, 2017

Uh oh!

Gregoor commented Mar 23, 2017

Uh oh!

Timer commented Mar 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifeiscontent commented Apr 10, 2017

Uh oh!

Timer commented Apr 10, 2017

Uh oh!

lifeiscontent commented Apr 10, 2017

Uh oh!

Timer commented Apr 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdaringe commented Apr 10, 2017

Uh oh!

quantuminformation commented Apr 12, 2017

Uh oh!

Timer commented Apr 12, 2017

Uh oh!

cdaringeApr 12, 2017

Choose a reason for hiding this comment

Uh oh!

cdaringeApr 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimerApr 12, 2017

Choose a reason for hiding this comment

Uh oh!

thg303 commented Apr 20, 2017

Uh oh!

quantuminformation commented Apr 20, 2017

Uh oh!

Timer commented Apr 20, 2017

Uh oh!

cdaringe commented Apr 28, 2017

Uh oh!

cdaringe commented Jan 7, 2017•
edited
Loading

tuchk4 commented Jan 7, 2017•
edited
Loading

EnoahNetzachJan 7, 2017•
edited
Loading

cdaringe commented Jan 7, 2017•
edited
Loading

Timer commented Mar 23, 2017•
edited
Loading

Timer commented Apr 10, 2017•
edited
Loading

cdaringeApr 12, 2017•
edited
Loading