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

Block on mobile unit tests fail and instructions on how to debug #18454

Merged
merged 10 commits into from Nov 15, 2019

Conversation

@hypest
Copy link
Contributor

hypest commented Nov 12, 2019

Description

This PR makes the native mobile tests Travis job necessary to succeed for the whole Travis run to "green".

How has this been tested?

Online via Travis itself on this PR and locally by running npm run test-unit:native:debug.

Types of changes

  1. Lift the "allow to fail" flagging in the Travis config for the native mobile tests job
  2. Add documentation on how to debug the native mobile unit tests
  3. Fix the test-unit:native:debug npm script to indeed wait for the node debugger to attach

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@hypest hypest added this to the Future milestone Nov 12, 2019
@hypest hypest requested a review from gziolo Nov 12, 2019
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 12, 2019

Works for me, but I'm not the best person to validate changes, @Tug maybe? :)

@Tug
Tug approved these changes Nov 12, 2019
Copy link
Contributor

Tug left a comment

Yep the --inspect-brk option should do the trick 👍
Nice addition to the documentation!

hypest added 4 commits Nov 12, 2019
Because there's also a Platform npm package installed too, running the
native mobile unit tests locally (at least on macOS) results in picking
up the wrong object. Putting the react-native/Libraries/Utilities first,
Jest manages to resolve the proper Platform import.

See
https://github.com/facebook/react-native/blob/v0.60.0/Libraries/react-native/react-native-implementation.js#L324-L326
for the import. The ambiguity seems fixed in RN v0.61.4 with
facebook/react-native@62c605e
@hypest hypest marked this pull request as ready for review Nov 13, 2019
@hypest hypest requested review from ajitbohra and chrisvanpatten as code owners Nov 13, 2019
test/native/jest.config.js Outdated Show resolved Hide resolved
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 13, 2019

There is also the Testing Overview document:
https://github.com/WordPress/gutenberg/blob/master/docs/contributors/testing-overview.md

We should either move the details about testing there and cross-reference or the other way around. This is really important so it's easy to find details about the native app.

@gziolo gziolo requested review from youknowriad and aduth Nov 13, 2019
@hypest

This comment has been minimized.

Copy link
Contributor Author

hypest commented Nov 13, 2019

There is also the Testing Overview document:
https://github.com/WordPress/gutenberg/blob/master/docs/contributors/testing-overview.md

We should either move the details about testing there and cross-reference or the other way around. This is really important so it's easy to find details about the native app.

Makes sense and I was thinking something along those line too 👍. Addressed with e5a0a97.

@gziolo
gziolo approved these changes Nov 13, 2019
Copy link
Member

gziolo left a comment

Thank you for working for all those additions to contributors guide ❤️

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 13, 2019

Nice, thanks, feel free to merge 👍

@hypest

This comment has been minimized.

Copy link
Contributor Author

hypest commented Nov 14, 2019

I'm going to leave this PR open for another day before merging. We brought this up more widely in the #core-editor weekly chat yesterday so, will leave some time for people to comment if any.

@hypest

This comment has been minimized.

Copy link
Contributor Author

hypest commented Nov 15, 2019

No new comments here so, will go ahead and merge.

@hypest hypest merged commit 8e2cd5b into master Nov 15, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@hypest hypest deleted the rnmobile/block-on-mobile-tests-fail branch Nov 15, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.0 Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.