Auth: Run codemods; use enzyme, remove mixin injection from tests #18472

ockham · 2017-10-03T11:17:57Z

Rationale: We're trying to get rid of mixin auto-injection for the React 16 upgrade, since this relies on methods (react/lib) that aren't part of the public interface.

This was tricky. These tests were a bit dubious in the first place, since they were arguably diluting the concept of a unit test. My fix was to use enzyme to shallow-render the component, which brings us closer to actual unit testing.

I also ran the i18n and createClass codemods on login.jsx, and modified it to export the un-localize()d Login component so we could test it (without having to inject the i18n mixin, which was the whole point of this PR). That's pretty standard stuff tho.

Other than that, I tried to change tests as little as possible. To continue to pass a callback to setState in tests, I had to upgrade enzyme (0dc7829):

2.5.0 supports passing callbacks to setState, which we require for client/auth/test/login.
Furthermore, there's a fix in 2.6.0 that's required for this to actually work
(enzymejs/enzyme#490)
Not updating to >=3.0.0 yet since chai-expect isn't compatible (yet).
Changelog: https://github.com/airbnb/enzyme/blob/master/CHANGELOG.md#252-november-9-2016

To test:

Verify that tests pass.
Verify that login still works.

matticbot · 2017-10-03T11:18:04Z

Test live: https://calypso.live/?branch=codemod/auth-login

gziolo

I haven't tested and scanned in-depth every line, but in overall those changes look really good 👍

Eslint complains about react/prefer-es6-class, but I think it's kind of excepted in this iteration because of mixins.

gziolo · 2017-10-03T14:25:18Z

client/auth/test/login.jsx


 describe( 'LoginTest', function() {
-	let Login, page, React, ReactDom, ReactClass, TestUtils;
-
-	before( () => {


We finally are getting rid of those require statements in before block :)

gziolo · 2017-10-03T14:26:14Z

package.json

@@ -247,7 +247,7 @@
    "chai": "3.5.0",
    "chai-enzyme": "0.5.2",
    "deep-freeze": "0.0.1",
-    "enzyme": "2.4.1",
+    "enzyme": "2.9.1",


As far as we can go with React 15.x 👍

Migrating to React 16 test wise might be a big challenge.

Think so? I was hoping to upgrade to enzyme 3.x once enzymejs/chai-enzyme#198 is merged, an a new chai-enzyme version that includes it is released...

I forgot about enzyme adapter for React 15. It might be solid. I haven’t tried it. We get some failing tests in Gutenberg when trying to use enzyme 3 with React 16. It looks like it has some hidden bugs.

You can also explore jest-enzyme 😃

gziolo · 2017-10-03T14:27:57Z

client/auth/login.jsx

-			</div>
-		);
-	}
+const LostPassword = localize( ( { translate } ) => {


Bonus points for moving those child components, to their own files 🥇

gziolo · 2017-10-03T19:11:39Z

Thanks for moving the components to their own files. Much appreciated 🏆

gziolo

This is good to go after shrinkwrap file gets regenerated with the latest changes from master. Great work @ockham 👍

tyxla

Gave this one a test run, looks and works well, awesome work 👍 As @gziolo mentioned, it should be good to go after generating a fresh shrinkwrap.

2.5.0 supports passing callbacks to `setState`, which we require for `client/auth/test/login`. Furthermore, there's a fix in 2.6.0 that's required for this to actually work (enzymejs/enzyme#490) Not updating to >=3.0.0 yet since chai-expect isn't compatible (yet). Changelog: https://github.com/airbnb/enzyme/blob/master/CHANGELOG.md#252-november-9-2016

These were only used by tests.

ockham · 2017-10-05T11:02:41Z

Thanks for reviewing @gziolo @tyxla!

…8472) * Auth: Prettify login.jsx * Auth: Run i18n codemod on login.jsx * Auth: Run react-create-class codemod on login.jsx * Auth: Prettify login.jsx * Auth: Simplify login.jsx * Auth: Named export for un-localized Login component * Framework: Upgrade enzyme to 2.9.1 2.5.0 supports passing callbacks to `setState`, which we require for `client/auth/test/login`. Furthermore, there's a fix in 2.6.0 that's required for this to actually work (enzymejs/enzyme#490) Not updating to >=3.0.0 yet since chai-expect isn't compatible (yet). Changelog: https://github.com/airbnb/enzyme/blob/master/CHANGELOG.md#252-november-9-2016 * Auth: Simplify FormTextInput ref (focus) * Auth: Use enzyme to test login.jsx * Auth: Remove obsolete refs from login.jsx (These were only used by tests.) * Auth: Move SelfHostedInstructions and LostPassword to separate files

ockham added Framework Login [Status] In Progress labels Oct 3, 2017

ockham self-assigned this Oct 3, 2017

ockham mentioned this pull request Oct 3, 2017

Framework: Remove i18n mixin injections from tests #18471

Merged

ockham force-pushed the codemod/auth-login branch from 75ac8a3 to 8361756 Compare October 3, 2017 11:22

ockham changed the title ~~Auth: Run i18n codemod on login.jsx~~ Auth: Run codemods; use enzyme, remove mixin injection from tests Oct 3, 2017

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 3, 2017

ockham requested review from jsnajdr, Tug, gziolo, sirreal and tyxla October 3, 2017 14:17

gziolo reviewed Oct 3, 2017

View reviewed changes

gziolo approved these changes Oct 5, 2017

View reviewed changes

tyxla approved these changes Oct 5, 2017

View reviewed changes

ockham added 9 commits October 5, 2017 11:57

Auth: Prettify login.jsx

b687272

Auth: Run i18n codemod on login.jsx

567598a

Auth: Run react-create-class codemod on login.jsx

af1bc2c

Auth: Prettify login.jsx

2ed77b2

Auth: Simplify login.jsx

6535c48

Auth: Named export for un-localized Login component

c53d690

Auth: Simplify FormTextInput ref (focus)

0a8f689

Auth: Use enzyme to test login.jsx

21e2cc5

ockham added 2 commits October 5, 2017 12:03

Auth: Remove obsolete refs from login.jsx

6454b21

These were only used by tests.

Auth: Move SelfHostedInstructions and LostPassword to separate files

0c8c7fb

ockham force-pushed the codemod/auth-login branch from b028f99 to 0c8c7fb Compare October 5, 2017 10:04

ockham merged commit 70aec10 into master Oct 5, 2017

ockham deleted the codemod/auth-login branch October 5, 2017 11:02

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth: Run codemods; use enzyme, remove mixin injection from tests #18472

Auth: Run codemods; use enzyme, remove mixin injection from tests #18472

ockham commented Oct 3, 2017 •

edited

Loading

matticbot commented Oct 3, 2017

gziolo left a comment

gziolo Oct 3, 2017

gziolo Oct 3, 2017

ockham Oct 3, 2017

gziolo Oct 3, 2017

gziolo Oct 3, 2017

ockham Oct 3, 2017 •

edited

Loading

gziolo commented Oct 3, 2017

gziolo left a comment

tyxla left a comment

ockham commented Oct 5, 2017

Auth: Run codemods; use enzyme, remove mixin injection from tests #18472

Auth: Run codemods; use enzyme, remove mixin injection from tests #18472

Conversation

ockham commented Oct 3, 2017 • edited Loading

matticbot commented Oct 3, 2017

gziolo left a comment

Choose a reason for hiding this comment

gziolo Oct 3, 2017

Choose a reason for hiding this comment

gziolo Oct 3, 2017

Choose a reason for hiding this comment

ockham Oct 3, 2017

Choose a reason for hiding this comment

gziolo Oct 3, 2017

Choose a reason for hiding this comment

gziolo Oct 3, 2017

Choose a reason for hiding this comment

ockham Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo commented Oct 3, 2017

gziolo left a comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

ockham commented Oct 5, 2017

ockham commented Oct 3, 2017 •

edited

Loading

ockham Oct 3, 2017 •

edited

Loading