Skip to content

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

Merged
hypest merged 10 commits into
masterfrom
rnmobile/block-on-mobile-tests-fail
Nov 15, 2019
Merged

Block on mobile unit tests fail and instructions on how to debug#18454
hypest merged 10 commits into
masterfrom
rnmobile/block-on-mobile-tests-fail

Conversation

@hypest

@hypest hypest commented Nov 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 12, 2019
@hypest hypest added this to the Future milestone Nov 12, 2019
@hypest hypest requested a review from gziolo November 12, 2019 11:07
@gziolo

gziolo commented Nov 12, 2019

Copy link
Copy Markdown
Member

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

@gziolo gziolo added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Nov 12, 2019
Tug
Tug approved these changes Nov 12, 2019
View reviewed changes

@Tug Tug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@hypest
Look in RN first so the Platform import picks up the correct one
75ae447 
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
react/react-native@62c605e
@hypest
Merge branch 'master' into rnmobile/block-on-mobile-tests-fail
98d4fc0
@hypest
Fix typo
8a10080
@hypest
Add additional way to launch the Chrome dev tools for mobile
3d165aa
@hypest hypest marked this pull request as ready for review November 13, 2019 09:12
@hypest hypest added the Needs Technical Feedback Needs testing from a developer perspective. label Nov 13, 2019
gziolo
gziolo reviewed Nov 13, 2019
View reviewed changes
Comment thread test/native/jest.config.js Outdated
@gziolo

gziolo commented Nov 13, 2019

Copy link
Copy Markdown
Member

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 aduth and youknowriad November 13, 2019 09:41
@hypest

hypest commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

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 reviewed Nov 13, 2019
View reviewed changes
Comment thread docs/contributors/testing-overview.md Outdated
gziolo
gziolo approved these changes Nov 13, 2019
View reviewed changes

@gziolo gziolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@gziolo

gziolo commented Nov 13, 2019

Copy link
Copy Markdown
Member

Nice, thanks, feel free to merge 👍

@hypest

hypest commented Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

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

hypest commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

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

@hypest hypest merged commit 8e2cd5b into master Nov 15, 2019
@hypest hypest deleted the rnmobile/block-on-mobile-tests-fail branch November 15, 2019 12:48
@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

Reviewers

@Tug Tug Tug approved these changes
@gziolo gziolo gziolo approved these changes
@ajitbohra ajitbohra Awaiting requested review from ajitbohra
@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten
@youknowriad youknowriad Awaiting requested review from youknowriad
@aduth aduth Awaiting requested review from aduth

Assignees

No one assigned

Labels

Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Needs Technical Feedback Needs testing from a developer perspective. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.

Projects

None yet

Milestone

Gutenberg 7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@hypest @gziolo @Tug @youknowriad