Integrate the mobile React Native testsuite with the main Gutenberg build #9883

hypest · 2018-09-13T19:59:43Z

Description

This PR adds the native mobile app repo as a git submodule and includes the mobile testsuite into the npm test script and the Travis test pipeline.

How has this been tested?

Locally doing npm test runs green.

To test:

Checkout the Gutenberg code into a new directory and cd into it
git submodule update --init --recursive to pull in the mobile submodule and its submodules
Follow the typical Gutenberg setup instructions
Tests should run green and also include the mobile tests. To confirm, look for the Setting RN platform to string in your terminal's ouput.

Types of changes

This PR doesn't introduce any code changes. All changes are done in the setup/tooling side of the project. In particular, the changes include:

Installing yarn, the package manager used by the mobile project. That's needed by the mobile npm scripts only.
The mobile repo has its own set of linting, compilation and run configurations so all those are ignored. .eslintignore got adjusted and .stylelinignore is introduced.
The test npm script adopts the test-mobile script to run after the Gutenberg unit tests
3 jobs are added to Travis: code correctness tests, Android codepath tests, iOS codepath tests
Gutenberg testsuites (unit, e2e) are ignoring the test modules inside the mobile folder since those need their own Jest configuration.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

The mobile code is assumed to be in the `gutenberg-mobile` subfolder.

The mobile project has its own lint rules and until the two projects can work off the same rules the main Gutenberg project needs to ignore it.

The mobile project still has Gutenberg as its own git submodule and so, many packages can be found in two places. Jest tests need to ignore the occurance of this second gutenberg sourcetree instance.

Instead, run `yarn install` earlier or in the same manner as `npm install`.

youknowriad · 2018-09-14T09:04:03Z

bin/install-node-nvm.sh

@@ -72,6 +72,21 @@ npm install
 # Make sure npm is up-to-date
 npm install npm -g

+# Check if yarn is installed
+if [ "$TRAVIS" != "true" ] && ! command_exists "yarn"; then


Curious why yarn is used for mobile? Can't we just make it "optional" and rely on npm consistently?

Yarn is the main packager used in React Native apps, at least that's what FB is using in the documentation and all.

We wanted to stay close to whatever is most popular in the React Native ecosystem so we keep our dev setup as "best practices" as possible.

That said, I think there is probably no special reason to stay with yarn down the road so, I could see us replacing that later and certainly when we integrate the 2 toolchains (web, RN) even more.

youknowriad

Nice addition, by looking at travis, it looks like the test are run in parallel and performant enought to include them.

I'd love some instructions on the testing documentation on how to run these locally... and how to potentially debug them if we notice failure.

hypest · 2018-09-14T16:50:04Z

I'd love some instructions on the testing documentation on how to run these locally... and how to potentially debug them if we notice failure.

For sure! I think I could add some instructions. To be honest, not sure where that testing documentation is so, any pointers will be appreciated :)

That said, could we defer adding that documentation to a follow up PR? I can work on that PR immediately but, it would be awesome if we merged this one as soon as we can. It seems that even as we speak, changes land to Gutenberg that break the mobile (recent one: a Dropdown that breaks the mobile build #9687 ) and it would be useful to avoid that now that we have this PR and works. WDYT @youknowriad ?

youknowriad

LGTM 👍 but I'd appreciate a second opinion before moving forward.

hypest · 2018-09-17T12:46:02Z

FWIW, added fecffe2 to pick up Riad's good suggestion to add some documentation on how to use or debug in case of mobile error.

aduth

There are some workflow issues here. I should expect to be able to run this without any issues, or at least very clear instructions on how to proceed (ideally with as few blockers as possible):

git clone https://github.com/WordPress/gutenberg.git
cd gutenberg
npm install
npm test

But there's a few issues:

We don't present the Yarn requirement at any point except through bin/install-node-nvm.sh. I don't use this workflow, so I'd expect others won't as well. Thus, when run a user will see an error sh: yarn: command not found .
Even if the user manages to infer their way to installing Yarn, we don't automate the submodule initialization, so the user is then presented with error Command "test:inside-gb" not found.
... I might have more commentary here, but even I couldn't figure out how I was supposed to get these tests actually running past this point. Running git submodule --init --recursive presents me with what appears to be an "invalid command" Git usage listing:

⇒ git submodule --init --recursive

usage: git submodule [--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
   or: git submodule [--quiet] status [--cached] [--recursive] [--] [<path>...]
   or: git submodule [--quiet] init [--] [<path>...]
   or: git submodule [--quiet] deinit [-f|--force] (--all| [--] <path>...)
   or: git submodule [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]
   or: git submodule [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
   or: git submodule [--quiet] foreach [--recursive] <command>
   or: git submodule [--quiet] sync [--recursive] [--] [<path>...]
   or: git submodule [--quiet] absorbgitdirs [--] [<path>...]

Even if there's some intention around removing the Yarn dependency, we should then at least make this opt-in (only run by direct command and in Travis) until that's a reality.

aduth · 2018-09-17T14:12:48Z

docs/reference/testing-overview.md

+```
+git submodule --init --recursive
+```
+You can then use the available script to lanuch the mobile testsuite in isolation:


Typo: "lanuch" -> "launch"

Fixed with 60a4935.

aduth · 2018-09-17T14:13:38Z

package.json

@@ -181,10 +181,11 @@
 		"publish:check": "npm run build:packages && lerna updated",
 		"publish:dev": "npm run build:packages && lerna publish --npm-tag next",
 		"publish:prod": "npm run build:packages && lerna publish",
-		"test": "npm run lint && npm run test-unit",
+		"test": "npm run lint && npm run test-unit && npm run test-mobile",


Can any of these be run concurrently? I seem to recall some race conditions previously which prevented some concurrent testing, but if test-mobile is meant to be isolated from other tests (at least for linting) maybe it's not problematic?

Can't really atest for lint and test-unit between them but yeah, test-mobile should be completely possible to run in parallel to the the rest.

Would you suggest I turn this into "test": "concurrently \"npm run lint && npm run test-unit\" \"npm run test-mobile\"" perhaps?

Yeah, that's what I'd have in mind.

Done with 0c5da64.

aduth · 2018-09-17T14:22:30Z

package.json

 		"pretest-e2e": "concurrently \"./bin/reset-e2e-tests.sh\" \"npm run build\"",
 		"test-e2e": "cross-env JEST_PUPPETEER_CONFIG=test/e2e/puppeteer.config.js wp-scripts test-unit-js --config test/e2e/jest.config.json --runInBand",
 		"test-e2e:watch": "npm run test-e2e -- --watch",
+		"test-mobile": "cd gutenberg-mobile; yarn test:inside-gb; cd ..;",


Have you checked that this works in Windows?

Hmm, no I haven't 😬

I'd be inclined to modify this into a single yarn --cwd gutenberg-mobile test:inside-gb command if you think it can be problematic in its current form.

hypest · 2018-09-17T16:23:10Z

Thanks for adding your review @aduth ! I've addressed your inline comments and here's some response to your awesome review comments:

There are some workflow issues here. I should expect to be able to run this without any issues:

git clone https://github.com/WordPress/gutenberg.git
npm install
npm test

But there's a few issues:

We don't present the Yarn requirement at any point except through bin/install-node-nvm.sh. I don't use this workflow, so I'd expect others won't as well. Thus, when run a user will see an error sh: yarn: command not found .

Aha, interesting. Not super familiar with the use cases of bin/install-node-nvm.sh but my understanding was that everyone would have been put through the initial setup for Gutenberg dev that involves bin/install-node-nvm.sh been ran anyway. In this case, installing yarn (globally) should be considered a step for that early setup phase.

What I can try and do here though is install yarn as a local dep, not a global package, and use it via npx. In that case, there will be no need for a manual installation of it other than the typical npm install. Would that be a better form of dependency?

Even if the user manages to infer their way to installing Yarn, we don't automate the submodule initialization, so the user is then presented with error Command "test:inside-gb" not found.

Indeed, as mentioned in the PR description, one would need to do that manually. What I can do here is to hook the submodule update to the preinstall script to be run automatically. Updating the submodule is super quick if it has been done once and the submodule ref hasn't changed so, there will be no performance impact at all to repeated use of the script. WDYT?

I might have more commentary here, but even I couldn't figure out how I was supposed to get these tests actually running past this point. Running git submodule --init --recursive presents me with what appears to be an "invalid command" Git usage listing:

Yeah, that's a 🤦‍♂️ error 😃. I forgot to add the update command to git there. I've updated the PR description to include it, sorry for the trouble 😬.

Even if there's some intention around removing the Yarn dependency, we should then at least make this opt-in (only run by direct command and in Travis) until that's a reality.

Gave it a quick try and it won't be trivial to remove the yarn dependency. So, for now, WDYT about adding it to devDependencies instead of a global install?

EDIT: I went ahead and made yarn a dev dependency with 5eaf120 so it is easier to assess.

* Yarn is installed locally as a development dependency * The mobile submodule is updated in preinstall * The mobile packages are installed in preinstall as well * Yarn is executed by providing the working (sub)directory as a parameter * No need to install yarn globally in Travis

hypest · 2018-09-18T06:13:20Z

Ready for another pass @aduth ! The "clone repo, npm install, npm test" routine should be available and running correctly, including the mobile tests. Let me know how that works for you, thanks!

aduth

It might be worth considering the size of Yarn as a dependency, but I'm content to prioritize the developer experience of the install. It didn't seem to bog down for me in any case.

Workflow feels much nicer now. Left one comment for your consideration.

aduth · 2018-09-18T16:09:45Z

test/unit/jest.config.json

@@ -25,5 +25,8 @@
 		"/test/e2e",
 		"<rootDir>/.*/build/",
 		"<rootDir>/.*/build-module/"
+	],
+	"modulePathIgnorePatterns": [
+		"<rootDir>/gutenberg-mobile/"


Should we include this for coveragePathIgnorePatterns ?

Oh, good point, seems like we should. Will look into it now.

Done with 24cd903.

hypest · 2018-09-18T17:02:03Z

It might be worth considering the size of Yarn as a dependency

Oh, I might not be well versed with this. Can you elaborate @aduth ? The yarn dependency is added as a devDependencies so I assume it won't affect the runtime bundle, no? Is it a problem even as a dev dependency?

In terms of how impactful can running it on every npm install can be, the repeated run is actually very very fast unless the mobile code or configuration changed, at which case it's normal to have it update whatever is needed anyway.

That said, if there is something more on your mind that is potentially a source of trouble please, do share so we pay attention.

but I'm content to prioritize the developer experience of the install. It didn't seem to bog down for me in any case.

Workflow feels much nicer now. Left one comment for your consideration.

Nice, good to hear! I'm sure we will keep an eye on keeping it good and better as we integrate more and more 👍

aduth · 2018-09-18T17:11:48Z

The yarn dependency is added as a devDependencies so I assume it won't affect the runtime bundle, no? Is it a problem even as a dev dependency?

In terms of how impactful can running it on every npm install can be, the repeated run is actually very very fast unless the mobile code or configuration changed, at which case it's normal to have it update whatever is needed anyway.

I wouldn't expect to be a concern for the runtime bundle, no. More in terms of cost as in the added time it takes to get a development environment up-and-running (the time of npm install). As mentioned, I think the workflow wins here are more than enough to offset any cost.

hypest · 2018-09-18T17:30:04Z

Thanks for the thorough reviews here all. Will go ahead and merge now, thanks!

aduth · 2018-10-26T14:23:48Z

That said, could we defer adding that documentation to a follow up PR?

Gave it a quick try and it won't be trivial to remove the yarn dependency. So, for now, WDYT about adding it to devDependencies instead of a global install?

There's implied to be some follow-up tasks here. Where can I track their progress?

Is the submodule intended to remain long-term? If not, which issue can I follow to track this progress? If so, how do we intend to remedy some of the workflow issues surrounding the submodule (showing as modified in local branch despite not being touched, "Find in Files" revealing duplicate copies from the submodule directory).

hypest · 2018-10-29T22:58:48Z

There's implied to be some follow-up tasks here. Where can I track their progress?

👋 @aduth , I had added some documentation with fecffe2 earlier in this PR. See native-mobile-testing and debugging-native-mobile.

Is the submodule intended to remain long-term?

Not really. But you know how these things go... "short term" can be long :). Joke aside, the current setup kinda helps us move forward but it's far from optimal. I just wished I had some time to at least try the git subtree idea I'm having and take it from there.

If not, which issue can I follow to track this progress?

None atm. Happy to accept suggestions on this one. What's the typical pattern we use in the Gutenberg repo to have such a ticket?

how do we intend to remedy some of the workflow issues surrounding the submodule (showing as modified in local branch despite not being touched

That's weird, how does that happen? Any steps available? FWIW, running the tests doesn't leave any trash behind to mark the submodule as dirty 🤔 . OTOH, changing to a different branch may leave the submodule un-updated, in which case npm install would fix it or if you manually update the submodules.

"Find in Files" revealing duplicate copies from the submodule directory

That should be relatively easy to fix. We just need to update all the nested (inside gutenberg-mobile) submodules except the gutenberg submodule, which is not needed in this setup. That should allieviate the duplicate results.

chrisvanpatten · 2018-10-29T23:09:11Z

OTOH, changing to a different branch may leave the submodule un-updated, in which case npm install would fix it or if you manually update the submodules.

It can also happen when running a git pull. npm install does update the submodule and fixes it up.

…enberg build (#9883)" This reverts commit cb908cf.

* Revert "Have Travis run mobile tests that use the parent code (#10034)" This reverts commit b1d9fb7. * Revert "Integrate the mobile React Native testsuite with the main Gutenberg build (#9883)" This reverts commit cb908cf.

hypest added 8 commits September 13, 2018 20:45

Install Yarn as part of the local setup

5cac2dc

Add scripts to run the mobile testsuite

05313b6

The mobile code is assumed to be in the `gutenberg-mobile` subfolder.

Add the native mobile app repo as a submodule

fe5a00a

Run the mobile testsuite as Travis jobs

04c425a

Ignore mobile subfolder when linting

75fb748

The mobile project has its own lint rules and until the two projects can work off the same rules the main Gutenberg project needs to ignore it.

Ignore the mobile subfolder to run Jest tests

398a414

The mobile project still has Gutenberg as its own git submodule and so, many packages can be found in two places. Jest tests need to ignore the occurance of this second gutenberg sourcetree instance.

yarn install shouldn't be ran as part of the test script

5ba0b20

Instead, run `yarn install` earlier or in the same manner as `npm install`.

Run the mobile testsuite as part of the test script

6db3b9c

hypest added [Status] In Progress Tracking issues with work in progress Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Sep 13, 2018

hypest requested review from youknowriad and gziolo September 13, 2018 19:59

hypest added 2 commits September 13, 2018 23:04

yarn install inside mobile when in Travis

88c641d

Ignore the mobile subfolder to run e2e Jest tests

91165d6

hypest added [Type] Build Tooling Issues or PRs related to build tooling and removed [Status] In Progress Tracking issues with work in progress labels Sep 13, 2018

hypest requested a review from tofumatt September 14, 2018 07:06

youknowriad reviewed Sep 14, 2018

View reviewed changes

youknowriad approved these changes Sep 17, 2018

View reviewed changes

youknowriad requested a review from a team September 17, 2018 08:03

Add some use and debugging instructions for mobile tests

fecffe2

aduth reviewed Sep 17, 2018

View reviewed changes

Fix typo

60a4935

hypest added 3 commits September 17, 2018 19:39

No need for npx. yarn is available to npm in scripts

551292a

No need for npx in postinstall either

924ced2

hypest added 2 commits September 17, 2018 20:05

Fix copy&paster error. Only update submodule in preinstall

5429e1c

Run the mobile tests in parallel for the test script

0c5da64

aduth approved these changes Sep 18, 2018

View reviewed changes

Add the mobile submodule to coverage ignore patterns

24cd903

hypest merged commit cb908cf into master Sep 18, 2018

hypest deleted the rnmobile/integrate-mobile-rn-testsuite branch September 18, 2018 17:30

mcsf mentioned this pull request Oct 15, 2018

Parser: Replace dynamic-block regex in do_blocks #10463

Closed

7 tasks

hypest mentioned this pull request Oct 24, 2018

Mobile test failures don't show at the end #10986

Closed

hypest added a commit that referenced this pull request Oct 31, 2018

Revert "Integrate the mobile React Native testsuite with the main Gut…

7295f0d

…enberg build (#9883)" This reverts commit cb908cf.

aduth mentioned this pull request Oct 31, 2018

Revert mobile RN testsuite integration #11318

Merged

4 tasks

hypest mentioned this pull request Nov 5, 2018

Plan for merging in the native mobile (React Native) app #11491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate the mobile React Native testsuite with the main Gutenberg build #9883

Integrate the mobile React Native testsuite with the main Gutenberg build #9883

hypest commented Sep 13, 2018 •

edited

Loading

youknowriad Sep 14, 2018

hypest Sep 14, 2018

youknowriad left a comment

hypest commented Sep 14, 2018 •

edited

Loading

youknowriad left a comment

hypest commented Sep 17, 2018 •

edited

Loading

aduth left a comment •

edited

Loading

aduth Sep 17, 2018

hypest Sep 17, 2018

aduth Sep 17, 2018

hypest Sep 17, 2018 •

edited

Loading

aduth Sep 17, 2018

hypest Sep 17, 2018

aduth Sep 17, 2018

hypest Sep 17, 2018 •

edited

Loading

hypest commented Sep 17, 2018 •

edited

Loading

hypest commented Sep 18, 2018

aduth left a comment

aduth Sep 18, 2018

hypest Sep 18, 2018

hypest Sep 18, 2018

hypest commented Sep 18, 2018

aduth commented Sep 18, 2018

hypest commented Sep 18, 2018

aduth commented Oct 26, 2018

hypest commented Oct 29, 2018

chrisvanpatten commented Oct 29, 2018

Integrate the mobile React Native testsuite with the main Gutenberg build #9883

Integrate the mobile React Native testsuite with the main Gutenberg build #9883

Conversation

hypest commented Sep 13, 2018 • edited Loading

Description

How has this been tested?

To test:

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

hypest commented Sep 14, 2018 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

hypest commented Sep 17, 2018 • edited Loading

aduth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

hypest commented Sep 17, 2018 • edited Loading

hypest commented Sep 18, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest commented Sep 18, 2018

aduth commented Sep 18, 2018

hypest commented Sep 18, 2018

aduth commented Oct 26, 2018

hypest commented Oct 29, 2018

chrisvanpatten commented Oct 29, 2018

hypest commented Sep 13, 2018 •

edited

Loading

hypest commented Sep 14, 2018 •

edited

Loading

hypest commented Sep 17, 2018 •

edited

Loading

aduth left a comment •

edited

Loading

hypest Sep 17, 2018 •

edited

Loading

hypest Sep 17, 2018 •

edited

Loading

hypest commented Sep 17, 2018 •

edited

Loading