refactor isomorphic tests #595

giladgray · 2017-02-01T19:46:52Z

Changes proposed in this pull request:

DRY isotest code by introducing helper function that does all the work
move usage into test/ directory in each package

giladgray · 2017-02-01T19:51:23Z

@codebling how can i be sure that these tests are actually passing and not reporting false positives? can you describe an easy way to make a component fail the test? (i'm an SSR noob and could use a hand.)

giladgray · 2017-02-01T19:52:31Z

test/isotest.js

        if (isReactClass(Component)) {
-            it("can be server-rendered", () => {
+            it(`<${ComponentKey}> can be server-rendered`, () => {


moved component name into test name so they can be easily distinguished when run in parallel (previously all its had the exact same name)

I haven't had a look at all of the changes yet, but in my experience, console outputs can become interlaced when running in parallel/asynchronously. I'd imagined this to be the reason why the tests were set up to run in series when I first found then.

blueprint-bot · 2017-02-01T22:14:52Z

revert circle.yml to run gulp test

Preview: docs | table
Coverage: core | datetime

for better reporting

giladgray · 2017-02-01T22:19:36Z

also @codebling is it isometric or isomorphic? I think the latter is correct, as in isomorphic-fetch.

adidahiya · 2017-02-01T22:26:03Z

✅ isomorphic

blueprint-bot · 2017-02-01T23:09:08Z

bring back missing require

Preview: docs | table
Coverage: core | datetime

codebling · 2017-02-02T01:13:23Z

@giladgray you can fail a test by attempting simply including a reference to document in the constructor (or any of the life cycle methods that run before DidMount, which never gets called on the server). document is undefined server-side

to produce static HTML, just as a server would. expecting this build to fail and reveal several broken components.

adidahiya · 2017-02-02T01:50:47Z

gulp/aliases.js

@@ -22,7 +22,8 @@ module.exports = (gulp) => {
    gulp.task("build", (done) => rs("clean", "compile", "webpack-compile-docs", done));

    // build code, run unit tests, terminate
-    gulp.task("test", ["test-dist", "karma", "isotest"]);
+    // NOTE: circle.yml runs each of these separately for better error reporting; be sure to sync


"be sure to sync"? what does that mean? also, how does it produce better error reporting? does it run the next gulp task even if a previous one fails?

means keep the two uses in sync. make sure they run the same tasks.

https://circleci.com/gh/palantir/blueprint/1503 shows two of the three failing.

i have an idea how to post comments on build failure, and we could post a different comment based on which ones fail.

adidahiya · 2017-02-02T01:52:44Z

test/isotest.js

+ * @param customProps       {{ [componentName: string]: any}}               custom props per component
+ * @param customChildren    {{ [componentName: string]: React.ReactNode }}  custom children per component
+ */
+module.exports = function isotest(Components, customProps, customChildren) {


can this be more verbose to make it clear that it generates multiple tests? generateIsoTests? same for the import name elsewhere

adidahiya · 2017-02-02T01:53:05Z

gulp/util/karma-config.js

            },
        },
        files: filesToInclude,
        frameworks: ["mocha", "chai", "phantomjs-shim", "sinon"],
+        mime: {


curious, why do we need this now?

it was added in #409, then lost in #381 when moving config to separate file (notice the smaller number 😄), and now restored here.

giladgray · 2017-02-02T01:54:58Z

@codebling got it. i tried messing with a few components and the tests didn't fail so i tried using enzyme's static HTML renderer and it revealed a handful of problematic components.

see build 1508 from commit above (edited) for some legit errors.

run in series so outputs don't clash. then we can just do the simple and maintainable `gulp test` in circle.yml instead of re-listing each task.

also rename all the vars

blueprint-bot · 2017-02-02T02:44:11Z

add required props and children

Preview: docs | table
Coverage: core | datetime

adidahiya · 2017-02-02T03:46:54Z

please file follow-up tickets for any skipped tests

codebling · 2017-02-02T03:47:13Z

@giladgray I see only passing tests in the build you linked. As far as switching to the Enzyme renderer, if you're able to find a way to pass specific Props and Children, then it should work. I wasn't able to find a way to do that with Enzyme (if I recall correctly), which is why I switched to ReactTestUtils. It's explained in the first comment in #381.

giladgray · 2017-02-02T05:42:47Z

@codebling ah you're right that link was wrong. i fixed it above. here's the correct build:
https://circleci.com/gh/palantir/blueprint/1508.

regarding the specific children: you'd already built all the pieces with customProps and customChildren! 🏆 I just filled them out. but that validations happens in the constructor, which I believe is supposed to run as part of the server render? it seemed to me like TestUtils wasn't doing enough work, or wasn't reporting errors, (false positives indeed) but enzyme immediately told me what was wrong.

Conflicts: gulp/isotest.js

blueprint-bot · 2017-02-03T19:33:23Z

Merge branch 'master' of github.com:palantir/blueprint into gg/isotest

Preview: docs | table
Coverage: core | datetime

cmslewis

[Timid approval] Looks good based on my very limited understanding of isomorphic stuff.

cmslewis · 2017-02-03T20:02:22Z

gulp/aliases.js

-    // build code, run unit tests, terminate
-    gulp.task("test", ["test-dist", "karma", "isotest"]);
+    // run test tasks in series to keep outputs separate
+    gulp.task("test", (done) => rs("test-dist", "karma", "isotest-mocha-w"));


Call done() here.

did it

blueprint-bot · 2017-02-03T20:11:19Z

call `done` in test alias

Preview: docs | table
Coverage: core | datetime

codebling · 2017-02-04T19:12:14Z

@giladgray regarding the build failures in the linked test, none of them seem specifically server-rendering related. I can't check out that commit any more because it's been squashed and I see that no changes were made to some of the failing components, so I wonder if it was an issue with the task. I see everything is running well on latest master (1dced45), so you must have figured it out. Great work !

Gilad Gray added 3 commits February 1, 2017 11:43

DRY isotest code

84b8c7e

move isotest files inside test/ dir

831fdeb

update alias usage

b21cb8e

giladgray added Domain: tooling Type: refactor labels Feb 1, 2017

giladgray commented Feb 1, 2017

View reviewed changes

revert circle.yml to run gulp test

8c07cf7

Gilad Gray added 2 commits February 1, 2017 14:16

use util/karma-config again

fb9ef5b

split out test tasks in circle.yml

9752cc6

for better reporting

Gilad Gray added 2 commits February 1, 2017 14:26

fix lint :(

5c4a14b

bring back missing require

8a6d942

switch to enzyme.render()

b9aa633

to produce static HTML, just as a server would. expecting this build to fail and reveal several broken components.

adidahiya reviewed Feb 2, 2017

View reviewed changes

Gilad Gray added 4 commits February 1, 2017 17:59

rename to generateIsomorphicTests

59f31cc

run-seq test tasks, run gulp test in circle.yml

307278a

run in series so outputs don't clash. then we can just do the simple and maintainable `gulp test` in circle.yml instead of re-listing each task.

add support for skipping components by name

381f95b

also rename all the vars

add required props and children

aa02019

giladgray added the P0 label Feb 3, 2017

Merge branch 'master' of github.com:palantir/blueprint into gg/isotest

887548c

Conflicts: gulp/isotest.js

cmslewis approved these changes Feb 3, 2017

View reviewed changes

cmslewis previously requested changes Feb 3, 2017

View reviewed changes

call done in test alias

ebc4261

leebyp approved these changes Feb 3, 2017

View reviewed changes

giladgray removed the P0 label Feb 3, 2017

giladgray merged commit 9af075c into master Feb 3, 2017

giladgray deleted the gg/isotest branch February 3, 2017 21:06

giladgray mentioned this pull request Feb 3, 2017

Prepare Release v1.8.0 #616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor isomorphic tests #595

refactor isomorphic tests #595

giladgray commented Feb 1, 2017 •

edited

giladgray commented Feb 1, 2017

giladgray Feb 1, 2017

codebling Feb 2, 2017

blueprint-bot commented Feb 1, 2017

giladgray commented Feb 1, 2017 •

edited

adidahiya commented Feb 1, 2017

blueprint-bot commented Feb 1, 2017

codebling commented Feb 2, 2017

adidahiya Feb 2, 2017

giladgray Feb 2, 2017

adidahiya Feb 2, 2017

adidahiya Feb 2, 2017

giladgray Feb 2, 2017 •

edited

giladgray commented Feb 2, 2017 •

edited

blueprint-bot commented Feb 2, 2017

adidahiya commented Feb 2, 2017

codebling commented Feb 2, 2017

giladgray commented Feb 2, 2017

blueprint-bot commented Feb 3, 2017

cmslewis left a comment

cmslewis Feb 3, 2017

blueprint-bot commented Feb 3, 2017

codebling commented Feb 4, 2017

refactor isomorphic tests #595

refactor isomorphic tests #595

Conversation

giladgray commented Feb 1, 2017 • edited

Changes proposed in this pull request:

giladgray commented Feb 1, 2017

giladgray Feb 1, 2017

Choose a reason for hiding this comment

codebling Feb 2, 2017

Choose a reason for hiding this comment

blueprint-bot commented Feb 1, 2017

revert circle.yml to run gulp test

giladgray commented Feb 1, 2017 • edited

adidahiya commented Feb 1, 2017

blueprint-bot commented Feb 1, 2017

bring back missing require

codebling commented Feb 2, 2017

adidahiya Feb 2, 2017

Choose a reason for hiding this comment

giladgray Feb 2, 2017

Choose a reason for hiding this comment

adidahiya Feb 2, 2017

Choose a reason for hiding this comment

adidahiya Feb 2, 2017

Choose a reason for hiding this comment

giladgray Feb 2, 2017 • edited

Choose a reason for hiding this comment

giladgray commented Feb 2, 2017 • edited

blueprint-bot commented Feb 2, 2017

add required props and children

adidahiya commented Feb 2, 2017

codebling commented Feb 2, 2017

giladgray commented Feb 2, 2017

blueprint-bot commented Feb 3, 2017

Merge branch 'master' of github.com:palantir/blueprint into gg/isotest

cmslewis left a comment

Choose a reason for hiding this comment

cmslewis Feb 3, 2017

Choose a reason for hiding this comment

blueprint-bot commented Feb 3, 2017

call `done` in test alias

codebling commented Feb 4, 2017

giladgray commented Feb 1, 2017 •

edited

giladgray commented Feb 1, 2017 •

edited

giladgray Feb 2, 2017 •

edited

giladgray commented Feb 2, 2017 •

edited