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

create flag to trigger --inspect-brk on test runner #2177

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

adkenyon
Copy link
Contributor

@adkenyon adkenyon commented Jul 2, 2020

  • This is a small change

Description:

I added a flag to allow attaching a debugger to the underlying test runner.

Usage:

detox test --debug-test-runner

Haven't updated the docs yet, I wanted to get some initial feedback before doing so.

@adkenyon adkenyon requested a review from rotemmiz as a code owner July 2, 2020 16:55
@noomorph
Copy link
Collaborator

noomorph commented Jul 13, 2020

@adkenyon , doesn't that workaround work?

nodejs/help#910

With Node.js 8, you can use NODE_OPTIONS='--inspect-brk' node foo.js

@d4vidi
Copy link
Collaborator

d4vidi commented Jul 15, 2020

CI failed due to a flaky test. I've restarted the associated downstream, stay tuned.
Anyways, where are on this?

@adkenyon
Copy link
Contributor Author

Haven't had the time to test @noomorph's suggestion. Hopefully will have some time by the end of the week

@adkenyon
Copy link
Contributor Author

@noomorph running NODE_OPTIONS='--inspect-brk' detox test -c ios.sim.debug didn't work for me. I could attach the debugger momentarily before the command would crash. Below is the following output, I checked for any other running debuggers. My guess is that the subprocesses created by detox test are all trying to debug on the same port

Debugger listening on ws://127.0.0.1:9229/7ad603d0-c5d7-4589-9dd4-3f0752c60973
For help, see: https://nodejs.org/en/docs/inspector
Debugger attached.
Debugger attached.
Debugger attached.
yarn run v1.22.4
$ detox test -c ios.sim.debug search-autocomplete
Starting inspector on 127.0.0.1:9229 failed: address already in use
error Command failed with exit code 12.```

@noomorph
Copy link
Collaborator

noomorph commented Jul 22, 2020

@adkenyon, okay, I see, thanks for checking.

Nevertheless, I would suggest renaming --debug-test-runner to --inspect-brk before we merge.
It would feel less nonstandard and more intuitive for people who spend time debugging Node.js projects from CLI.

detox test --inspect-brk

Also, please check that it works okay with multiple Jest workers because I am really not sure. If it does not, better force -w to be 1.

@noomorph noomorph merged commit 3f75088 into wix:master Jul 23, 2020
@adkenyon
Copy link
Contributor Author

@noomorph wasn't quite ready yet. Still needed to test out the workers flag. I'll follow up if there's an issue

@noomorph
Copy link
Collaborator

noomorph commented Jul 23, 2020

@adkenyon, well, I think there's a minor issue with it when there are multiple workers — it does not really work. 😆 But does not break anything either, seemingly. I've checked manually the latter — I specified multiple workers and tried some debugging which did not work out but never crashed, so that was sufficient for me.

You are welcome to submit one more PR for improving user experience, that exits with an error if you specify simultaneously multiple workers and --inspect-brk. Or it could be forcing --workers 1, as an alternative, with some warning message.

@d4vidi , @rotemmiz , @LeoNatan any ideas what is a better behavior?
Should we force workers to 1 with the debug flag or just fail immediately when debugging with multiple workers — since it does not work well anyway from Node.js side... 🤷‍♂️

@adkenyon adkenyon deleted the debug-test-runner-flag branch July 23, 2020 15:27
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

3 participants