feature/add-first-class-debugging-support-for-tests #1360

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

packages/react-scripts/bin/react-scripts.js

+  var scriptArgs = [require.resolve('../scripts/' + script)].concat(args);
+  var debugFlag = getDebugFlag(args);
+  if (debugFlag) {
+    scriptArgs.unshift(debugFlag);


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

packages/react-scripts/scripts/test.js

+ * Greetings! If you are here attempting to start a debugging session, please
+ * ensure that your debugger of choice is configured to enable source maps,
+ * otherwise your code may appear mangled by babel!
+ */


🎉 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

packages/react-scripts/scripts/test.js

+  if (!isRunInBand) {
+    argv.push('--runInBand')
+  }
+  argv.splice(argv.indexOf(debugFlag), 1)


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

.gitignore

@@ -7,3 +7,4 @@ template/src/__tests__/__snapshots__/
 lerna-debug.log
 npm-debug.log
 /.changelog
+.vscode


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

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

packages/react-scripts/bin/react-scripts.js

@@ -1,5 +1,6 @@
 #!/usr/bin/env node
 var spawn = require('cross-spawn');
+var getDebugFlag = require('../utils/getDebugFlag');


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

packages/react-scripts/config/jest/babelTransform.js

-  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

packages/react-scripts/utils/createJestConfig.js

-
+  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

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 14d53fb Compare January 7, 2017 20:49

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

cdaringe changed the title ~~feature/add-first-class-debugging-support~~ feature/add-first-class-debugging-support-for-tests Jan 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

lock bot locked and limited conversation to collaborators Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/add-first-class-debugging-support-for-tests #1360

feature/add-first-class-debugging-support-for-tests #1360

cdaringe commented Jan 7, 2017 •

edited

Loading

facebook-github-bot commented Jan 7, 2017

cdaringe Jan 7, 2017

cdaringe Jan 7, 2017

facebook-github-bot commented Jan 7, 2017

cdaringe Jan 7, 2017

cdaringe commented Jan 7, 2017

tuchk4 commented Jan 7, 2017 •

edited

Loading

EnoahNetzach Jan 7, 2017 •

edited

Loading

EnoahNetzach Jan 7, 2017

cdaringe commented Jan 7, 2017 •

edited

Loading

cdaringe commented Jan 9, 2017

gaearon commented Feb 12, 2017

cdaringe commented Feb 12, 2017

Gregoor commented Mar 23, 2017

Timer commented Mar 23, 2017 •

edited

Loading

lifeiscontent commented Apr 10, 2017

Timer commented Apr 10, 2017

lifeiscontent commented Apr 10, 2017

Timer commented Apr 10, 2017 •

edited

Loading

cdaringe commented Apr 10, 2017

quantuminformation commented Apr 12, 2017

Timer commented Apr 12, 2017

cdaringe Apr 12, 2017

cdaringe Apr 12, 2017 •

edited

Loading

Timer Apr 12, 2017

thg303 commented Apr 20, 2017

quantuminformation commented Apr 20, 2017

Timer commented Apr 20, 2017

cdaringe commented Apr 28, 2017

avantgardnerio commented Aug 7, 2017

cdaringe commented Aug 7, 2017

avantgardnerio commented Aug 8, 2017

feature/add-first-class-debugging-support-for-tests #1360

feature/add-first-class-debugging-support-for-tests #1360

Conversation

cdaringe commented Jan 7, 2017 • edited Loading

problem statement

solution

facebook-github-bot commented Jan 7, 2017

cdaringe Jan 7, 2017

Choose a reason for hiding this comment

cdaringe Jan 7, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Jan 7, 2017

cdaringe Jan 7, 2017

Choose a reason for hiding this comment

cdaringe commented Jan 7, 2017

tuchk4 commented Jan 7, 2017 • edited Loading

EnoahNetzach Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

EnoahNetzach Jan 7, 2017

Choose a reason for hiding this comment

cdaringe commented Jan 7, 2017 • edited Loading

cdaringe commented Jan 9, 2017

gaearon commented Feb 12, 2017

cdaringe commented Feb 12, 2017

Gregoor commented Mar 23, 2017

Timer commented Mar 23, 2017 • edited Loading

lifeiscontent commented Apr 10, 2017

Timer commented Apr 10, 2017

lifeiscontent commented Apr 10, 2017

Timer commented Apr 10, 2017 • edited Loading

cdaringe commented Apr 10, 2017

quantuminformation commented Apr 12, 2017

Timer commented Apr 12, 2017

cdaringe Apr 12, 2017

Choose a reason for hiding this comment

cdaringe Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Timer Apr 12, 2017

Choose a reason for hiding this comment

thg303 commented Apr 20, 2017

quantuminformation commented Apr 20, 2017

Timer commented Apr 20, 2017

cdaringe commented Apr 28, 2017

avantgardnerio commented Aug 7, 2017

cdaringe commented Aug 7, 2017

avantgardnerio commented Aug 8, 2017

cdaringe commented Jan 7, 2017 •

edited

Loading

tuchk4 commented Jan 7, 2017 •

edited

Loading

EnoahNetzach Jan 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

cdaringe Apr 12, 2017 •

edited

Loading