Fix browser tests (Karma + Webpack setup). #1109

joaovieira · 2018-04-08T00:15:28Z

TD;LR
This has the same problem as enzymejs/enzyme#1318. Solved using enzymejs/enzyme#1318 (comment).

This also:

fixes several PropTypes and Sinon warnings
organizes the mocha setup in a single place - mocha.opts
removes browser tests from "allowed failures"

Explanation
When using a files array, each file is fed into the webpack config creating an isolated bundle. When a bundle executes it is independent from other bundles (i.e. even if we load tests/enzymeSetup.js it only sets up the adapter for that bundle) - or so it seems.

Having a single bundle is also more efficient as webpack only needs to build once and there's no duplication.

The new entrypoint file is loading the tests the same way as mocha:

load all files in test/_helpers
load all *_spec files in tests

coveralls · 2018-04-08T00:21:23Z

Coverage remained the same at 86.457% when pulling 0a07a3e on joaovieira:fix-browser-tests into b52908d on airbnb:master.

joaovieira · 2018-04-08T00:51:20Z

test/components/DateRangePicker_spec.jsx

@@ -18,6 +18,8 @@ import {
 const requiredProps = {
  onDatesChange: () => {},
  onFocusChange: () => {},
+  startDateId: 'startDate',
+  endDateId: 'endDate',


joaovieira · 2018-04-08T00:51:52Z

test/components/DateRangePicker_spec.jsx

@@ -316,7 +318,6 @@ describe('DateRangePicker', () => {
        const wrapper = shallow((
          <DateRangePicker
            {...requiredProps}
-            onDateChange={sinon.stub()}


not used; also onDateChange does not exist for DateRangePicker anymore

joaovieira · 2018-04-08T00:52:13Z

test/components/DayPickerKeyboardShortcuts_spec.jsx

@@ -117,7 +117,7 @@ describe('DayPickerKeyboardShortcuts', () => {
        });

        afterEach(() => {
-          openKeyboardShortcutsPanelStub.reset();
+          openKeyboardShortcutsPanelStub.resetHistory();


sinon.reset() has been deprecated

ljharb

Thanks, this is amazing. Just the one comment.

ljharb · 2018-04-09T06:44:03Z

test/mocha.opts

@@ -2,3 +2,4 @@
 --compilers js:babel-register,jsx:babel-register
 --require airbnb-js-shims
 --recursive
+test/_helpers test/**/*_spec*


This file needs a trailing newline; separately, the spec dir shouldn’t go here - i should be able to run mocha path/to/file and not run every test (and I’d prefer ever file inside “test” get treated as a spec, not just ones following a naming convention)

joaovieira · 2018-04-09T10:31:53Z

test/_helpers/ignoreSVGStrings.jsx

@@ -1,6 +0,0 @@
-// Don't load svg strings into tests
-require.extensions['.svg'] = (obj) => {


require.extensions is node.js specific (does not work in webpack/browser). Though, it also seems it's not necessary anymore as enzyme fixed support for SVGs (enzymejs/enzyme#375)?

The enzyme bug was on svg elements being present; this file makes svg files requirable in node, which is a different thing.

Thanks for clarifying. Then isn't what this is doing as well https://github.com/airbnb/react-dates/blob/master/.babelrc#L6? The .svg imports won't get to node.js - they'll be transformed before that in babel.

I think we weren't using the inline-svg plugin when this was first written so it's probable that it's no longer necessary.

joaovieira · 2018-04-09T10:32:47Z

test/browser-main.js

@@ -0,0 +1,6 @@
+const requireAll = requireContext => requireContext.keys().forEach(requireContext);
+
+if (typeof window !== 'undefined') {


I'd rather use Mocha's --exclude option (mochajs/mocha#3210) once it lands instead of this.

Mocha has dropped older node versions so we shouldn’t be upgrading too far on it; this approach is fine.

Fair enough 👍

Edit: Just checking. Mocha supports node >= 4. react-dates has dependencies that only support node >= 4 as well (enzyme-adapter-react-helper > install-peerdeps > has-yarn) - which is causing the iojs tests to fail https://travis-ci.org/airbnb/react-dates/jobs/363589154#L4426).

that is true; but that is something i'm still working on fixing :-)

majapw

This is really really great! :D Thank you so much for this clean-up.

majapw · 2018-04-09T18:19:01Z

test/_helpers/ignoreSVGStrings.jsx

@@ -1,6 +0,0 @@
-// Don't load svg strings into tests
-require.extensions['.svg'] = (obj) => {


I think we weren't using the inline-svg plugin when this was first written so it's probable that it's no longer necessary.

joaovieira added 2 commits April 8, 2018 00:55

Use single entrypoint for karma when using webpack. Fix warnings.

25b4718

Merge branch 'master' into fix-browser-tests

82abb71

Move mocha setup to mocha.opts. Fix coverage. Force browser tests.

89986db

joaovieira commented Apr 8, 2018

View reviewed changes

joaovieira mentioned this pull request Apr 8, 2018

Add appendToBody and disableScroll props #1069

Merged

ljharb requested changes Apr 9, 2018

View reviewed changes

ljharb requested a review from majapw April 9, 2018 06:45

joaovieira force-pushed the fix-browser-tests branch from eb9ba62 to 89986db Compare April 9, 2018 10:25

Revert mocha setup. Only run browser entry point if in browser.

0a07a3e

joaovieira commented Apr 9, 2018

View reviewed changes

majapw approved these changes Apr 9, 2018

View reviewed changes

ljharb approved these changes Apr 9, 2018

View reviewed changes

majapw merged commit d18da3b into react-dates:master Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix browser tests (Karma + Webpack setup). #1109

Fix browser tests (Karma + Webpack setup). #1109

joaovieira commented Apr 8, 2018 •

edited

Loading

coveralls commented Apr 8, 2018 •

edited

Loading

joaovieira Apr 8, 2018

joaovieira Apr 8, 2018

joaovieira Apr 8, 2018

ljharb left a comment

ljharb Apr 9, 2018

joaovieira Apr 9, 2018

ljharb Apr 9, 2018

joaovieira Apr 9, 2018 •

edited

Loading

majapw Apr 9, 2018

joaovieira Apr 9, 2018

ljharb Apr 9, 2018

joaovieira Apr 9, 2018 •

edited

Loading

ljharb Apr 9, 2018

majapw left a comment

majapw Apr 9, 2018

		@@ -1,6 +0,0 @@
		// Don't load svg strings into tests
		require.extensions['.svg'] = (obj) => {

		@@ -0,0 +1,6 @@
		const requireAll = requireContext => requireContext.keys().forEach(requireContext);

		if (typeof window !== 'undefined') {

Fix browser tests (Karma + Webpack setup). #1109

Fix browser tests (Karma + Webpack setup). #1109

Conversation

joaovieira commented Apr 8, 2018 • edited Loading

coveralls commented Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb 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

joaovieira Apr 9, 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

joaovieira Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaovieira commented Apr 8, 2018 •

edited

Loading

coveralls commented Apr 8, 2018 •

edited

Loading

joaovieira Apr 9, 2018 •

edited

Loading

joaovieira Apr 9, 2018 •

edited

Loading