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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run native tests in Gutenberg CI #16404

Merged
merged 14 commits into from Jul 11, 2019

Conversation

@etoledom
Copy link
Contributor

commented Jul 3, 2019

fix wordpress-mobile/gutenberg-mobile#1214

Description

This is an experimental PR to run the native test suite in Gutenberg CI.

Motivation

This is a step to bring gutenberg web and mobile together, by having an "early alert system" for changes that might break the native implementation.
This new CI job is not meant to be required to pass in order to merge a PR, but it will alert the mobile team of breaking changes as soon as one is detected giving us more time to react to it.

This is also meant to be an intermediated step before merging both project as a mono-repo. Since the mono-repo might take a longer time, we would like to have this early alert system as soon as possible.

Issues

There is one problem that I haven't been able to figure out yet, and is that the babel.config.js file is quite different between web and mobile. I'd like to ask for help figuring out how to handle this problem.

I committed the mobile version of the config. Commented is the original version, and a mixed version I tried using the overrides options without success.

EDIT: Issues have been solved 馃帀

Notes

Most of the current mobile tests are unit test, isolated from gutenberg-specific code. This won't really help us just yet with the end goal of this PR. The idea is to create integration tests between the mobile js code and gutenberg-specific code (still keeping the native part mocked out), starting by the most critical parts like RichText.

@etoledom etoledom self-assigned this Jul 3, 2019

package.json Outdated Show resolved Hide resolved

@etoledom etoledom requested review from gziolo, aduth, youknowriad, hypest, koke and Tug Jul 3, 2019

@koke

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

There is one problem that I haven't been able to figure out yet, and is that the babel.config.js file is quite different between web and mobile. I'd like to ask for help figuring out how to handle this problem.

Looking at Babel's documentation:

Babel has as concept of a "root" directory, which defaults to the current working directory

On a first try, the tests aren't passing for me, but I was able to have a specific config by putting ours in test/native/babel.config.js and then edit package.json to change to that directory before running the tests:

"test-unit:native": "cd ./test/native && cross-env NODE_ENV=test jest --config ./jest.config.js"

You can add a console.warn('馃檵鈥 Native/Web config'); at the top of each config file to see which one Jest is picking up.

@koke

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I get the tests passing but I'm seeing this at the start:

Setting RN platform to: default (android)
jest-haste-map: duplicate manual mock found: @wordpress/block-library
  The following files share their name; please delete one of them:
    * <rootDir>/test/unit/__mocks__/@wordpress/block-library.js
    * <rootDir>/test/native/__mocks__/@wordpress/block-library.js

And then this at the end:

Ran all test suites.
  console.error node_modules/react/cjs/react.development.js:188
    Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: object. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.

  console.error node_modules/react/cjs/react.development.js:188
    Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: object. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
@koke

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I was a bit confused by the empty mock files, but looking at the docs I guess that's how it's done 馃し鈥嶁檪

babel.config.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@koke thanks for the review!

About #16404 (comment) , I haven't yet getting into the warnings, I was happy enough that the tests were passing 馃帀

I did an update from master, and there are new tests that trigger a lot more of those warnings at the end. I'll definitely look into it.

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@youknowriad - Updated with @koke's recommendation to split babel.config file.
Now all tests (both test-unit and test-unit:native ) are green on CI.

Do you see anything else on this PR that would be a blocker to merge?

My remaining steps are:

  • Solve warnings on native tests
  • Make test-unit:native not required to merge a PR on gutenberg

After these are ready, I'll remove the Draft label.

@etoledom etoledom marked this pull request as ready for review Jul 9, 2019

@etoledom etoledom force-pushed the try/run-native-tests branch from df51ac7 to 67622e0 Jul 9, 2019

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

I believe this PR is ready for a formal Review:

  • All tests are passing.
  • Solved all warnings on native tests.
  • Removed some dependencies that weren't necessary.
  • Set native tests as "allowed to fail"
  • Native tests has their own babel config inside tests/native/

Next: Add some more meaningful integration tests between *.native.js files and gutenberg code. This will help us achieve the original idea of early warning when gutenberg changes might break native implementation. I plan to do this on a next PR.

@youknowriad @Tug @gziolo @hypest @aduth I'd love to know what do you think about this.
Hopefully we can merge this soon as a first step and continue improve this implementation on the way.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Any particular reason fo use the "allowed failures" section for the job?

@youknowriad
Copy link
Contributor

left a comment

Looks good to me overall. My main concern is the extra dependencies but we can see how impactful it is overtime.

I'd appreciate other 馃憖

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Thank you @youknowriad for the review!

Any particular reason fo use the "allowed failures" section for the job?

The idea is to not block a gutenberg PR if a mobile tests fails, but to alert us so we can react to it timely.
I'd say let's keep it as allowed to fail for some time as a test ground. Then we can require it if everything is working properly. (cc @hypest )

package.json Outdated Show resolved Hide resolved
test/native/jest.config.js Outdated Show resolved Hide resolved
test/native/jest_gb.config.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -65,9 +65,7 @@
},
"devDependencies": {
"@babel/core": "7.4.4",
"@babel/plugin-proposal-async-generator-functions": "7.2.0",

This comment has been minimized.

Copy link
@Tug

Tug Jul 10, 2019

Contributor

Did you run npm i afterwards? It should have updated package-lock.json I believe

This comment has been minimized.

Copy link
@etoledom

etoledom Jul 10, 2019

Author Contributor

I did. I was expecting changes on package-lock.json too but it didn't modify it.

This comment has been minimized.

Copy link
@etoledom

etoledom Jul 10, 2019

Author Contributor

I removed them with npm uninstall, then run npm install just to be sure 炉_(銉)_/炉

@Tug

Tug approved these changes Jul 10, 2019

Copy link
Contributor

left a comment

LGTM 馃憤
I had a few comments about extra entries in package.json but it seems you already started addressing those...

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Thank you both @youknowriad and @Tug for taking a look.
All comments were addressed.

With both I'll merge this PR.

@aduth

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Any particular reason fo use the "allowed failures" section for the job?

The idea is to not block a gutenberg PR if a mobile tests fails, but to alert us so we can react to it timely.

This seems reasonable to me, but logistically I'm curious how you plan to stay on top of these? Are there notifications which you can subscribe to?

(Don't let this comment imply any hesitancy to a merge)

@etoledom etoledom force-pushed the try/run-native-tests branch from 94f816d to 9e7aad1 Jul 11, 2019

.travis.yml Outdated Show resolved Hide resolved

@etoledom etoledom merged commit 9aab86d into master Jul 11, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.2 milestone Jul 11, 2019

@etoledom

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

how you plan to stay on top of these? Are there notifications which you can subscribe to?

@aduth - We don't have it clear yet, and we are open to suggestions! This PR by itself is already a very big step for us :)

@etoledom etoledom deleted the try/run-native-tests branch Jul 11, 2019

Tug added a commit that referenced this pull request Jul 12, 2019

Run native tests in Gutenberg CI (#16404)
* Added needed rn dependencies to run native tests

* Adding native test's config files

* Remove unnecessary mock files from test

* Add native mocks

* Added commands to run native tests

* Fix native editor tests

* Add native tests to Travis as allow_failures

* Added native unit test as travis job

* Experiment - Add native tests as allow_failures

* Clean up native jest.config files

* Remove dependencies not needed from package.json

* Remove more dependencies not needed from package.json

* Adding some missing native mocks

* Renamed travis native tests job to `JavaScript native mobile tests`
},
"brace-expansion": {
"version": "1.1.11",
"bundled": true,
"dev": true,
"optional": true,

This comment has been minimized.

Copy link
@aduth

aduth Jul 12, 2019

Member

I'm guessing you installed these new dependencies using an older version of NPM. These changes shouldn't be here, and are causing local changes when running npm install from a fresh clone. Make sure to run the latest version of NPM when developing in Gutenberg (reference).

This comment has been minimized.

Copy link
@aduth

aduth Jul 12, 2019

Member

and are causing local changes when running npm install from a fresh clone

This is addressed in #16567

This comment has been minimized.

Copy link
@etoledom

etoledom Jul 12, 2019

Author Contributor

Thanks for the heads-up!
Updated my local npm version to avoid this in the future.

Sorry for the inconvenience 馃檹

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Run native tests in Gutenberg CI (WordPress#16404)
* Added needed rn dependencies to run native tests

* Adding native test's config files

* Remove unnecessary mock files from test

* Add native mocks

* Added commands to run native tests

* Fix native editor tests

* Add native tests to Travis as allow_failures

* Added native unit test as travis job

* Experiment - Add native tests as allow_failures

* Clean up native jest.config files

* Remove dependencies not needed from package.json

* Remove more dependencies not needed from package.json

* Adding some missing native mocks

* Renamed travis native tests job to `JavaScript native mobile tests`

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Run native tests in Gutenberg CI (WordPress#16404)
* Added needed rn dependencies to run native tests

* Adding native test's config files

* Remove unnecessary mock files from test

* Add native mocks

* Added commands to run native tests

* Fix native editor tests

* Add native tests to Travis as allow_failures

* Added native unit test as travis job

* Experiment - Add native tests as allow_failures

* Clean up native jest.config files

* Remove dependencies not needed from package.json

* Remove more dependencies not needed from package.json

* Adding some missing native mocks

* Renamed travis native tests job to `JavaScript native mobile tests`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.