Jest spike #66

Droogans · 2017-10-30T21:48:00Z

JIRA: https://jira.rax.io/browse/SURF-667

I'm pretty happy with this. I can do more polish here, but personally I'd rather just the core of this reviewed for now and address it during review.

Since jest is a pretty nice test runner, I decided to port over the visreg tests to it. This drops mocha and chai since otherwise we'd have two testing libraries used in the same project, which is not ideal.

These are kind of like unit tests, but they have a few differences that might be new to the team.

They use chrome, not karma (and, soon...chrome headless)
They use selenium, but it could technically use jsdom, though there'd be a lot more boilerplate
They don't live inside the source/ directory, as jest doesn't like colocated tests

Because of the above three, I'd like to propose branding these as functional tests. Unit testing can be sorted out later once components of more substance are crafted. Currently, my working mental model of "unit" testing will include extracting out pure (read: node-friendly) classes and logic into their own modules and interleaving them into the source code with some kind of a build tool (likely webpack).

That would make it very pleasant to test. Require the module under test, instantiate the class containing business logic, and finally, pass in arguments to pure functions. Once you've verified that the outputs match what is expected, you're done. Afterwards, you can use that foundational business logic class in the html, extend it with HTMLElement like we do today, and let it do it's thing.

From there, functional tests will ensure that interacting with it in the browser works as expected, with visreg as a final stop gap for measuring style changes over time.

LGTM's

Dev LGTM
Zoom LGTM

CITguy · 2017-10-31T14:49:28Z

test/index.ts

-    });
-});
+export function visregSuite(browserName: string) {
+    jasmine.DEFAULT_TIMEOUT_INTERVAL = 60e3;


Use 60000 instead of 60e3. It's easier to grok.

CITguy · 2017-10-31T14:50:55Z

test/chrome.ts

@@ -0,0 +1,3 @@
+import * as tests from "./index";


Seems index is more of a utility rather than an entry point. Perhaps we should rename index.ts to something more appropriate. Which would also warrant a rename of the import alias.

CITguy · 2017-10-31T15:18:36Z

test/package.json

@@ -20,15 +18,24 @@
  },
  "scripts": {
    "build": "tsc",
+    "build:visreg": "tsc bin/visreg.ts bin/visreg.config.ts --outdir built/bin --lib es6 && tsc",


Clean this up

CITguy · 2017-10-31T15:19:33Z

test/package.json

    "webdriver": "npm run webdriver:update && npm run webdriver:start",
    "webdriver:update": "scripts/webdriver-update",
    "webdriver:start": "scripts/webdriver-start"
+  },
+  "jest": {


Move this somewhere else.

CITguy · 2017-10-31T15:25:15Z

test/util.ts

@@ -31,6 +32,6 @@ export function $x(
 }

 // "page object"


Document this better.

CITguy · 2017-10-31T15:28:46Z

Future request: plug in tests into bin/start.js so that tests run automatically when devs run yarn start

cathysiller

Dev LGTM

Droogans · 2017-10-31T21:14:06Z

@CITguy Made https://jira.rax.io/browse/SURF-703 to track the watch mode for functional tests.

This will be much simpler with "proper" unit tests, which is a work in progress.

Droogans · 2017-11-01T19:41:46Z

@CITguy this is refactored and looking better.

However, the more time I spend with jest the more convinced I am that it won't make sense to run selenium tests with it. I'm willing to entertain it for a while and see how it goes, but my recommendation currently is to consider this spike conclusive: jest is not a good fit for traditional selenium-based testing.

I think it would work better for unit tests, since jest is designed to be slow, but smart and only run tests against things that have changed.

CITguy · 2017-11-01T20:01:52Z

test/tsconfig.json

-    ],
-    "exclude": [
-        "node_modules"
+        "common/*.ts",


should this be common/**/*.ts to match files in common/util/?

It's just a single file in there right now. util.ts.

Droogans · 2017-11-01T20:40:54Z

Ok I can make things much better for now by just cutting out firefox.

Firefox + jest == not a good time. Chrome is alright. I'll get to the root of the issue as I spend more time on it, but I have to move on for now.

Droogans · 2017-11-02T13:21:02Z

Alright so I've made a lot of progress here.

Firefox is slow because it takes up to 30 seconds to receive the "terminate" signal at the end of testing. The other slowness of the tests appears in both Chrome and Firefox, using the following set up. Reproduction steps include:

Use jest (this doesn't happen with mocha)
Delete your screenshots directory (when a previous screenshot exists, this doesn't happen)
Take a full page screenshot

For some reason, pngjs.PNG.sync.write(this.png); can take up to two minutes to run when there is no baseline. I'm working on profiling the calls and digging into our source modules to find out what could possibly be causing this, since it appears to only happen in the context of jest.

Could it have something to do with image comparison speeding up a later operation via some kind of caching? Could it be a memory issue with jest's optimization strategies? There's a lot going on here but fortunately I have a couple of solid work arounds that will allow us to at least move on.

Use Chrome
Don't take full-page screen shots.

Droogans · 2017-11-02T14:11:29Z

I've also discovered that it's only the first full size screenshot that does this. Subsequent full size screenshots don't take 10x less than the first one!

CITguy · 2017-11-02T20:32:58Z

test/built/functional/hx-reveal.js.md

+
+> Snapshot 1
+
+    `<hx-reveal>␊


Can we configure the newline characters in AVA?

nm, I just realized that's on purpose to visualize new lines in the documentation

Droogans · 2017-11-02T20:59:57Z

Feature request: dig through html source on disk during a before-all phase and find any data-visreg attrs on tags to generate visreg test cases.

Droogans · 2017-11-02T21:01:12Z

test/package.json

@@ -18,16 +18,20 @@
  "scripts": {
    "build": "tsc",
    "clean": "npm run clean:visreg && npm run clean:func",
-    "clean:func": "find functional/built -mindepth 1 \\( ! -path 'functional/built/functional/__snapshots__/*' \\) -delete ",
-    "clean:visreg": "rm -rf visreg/screenshots visreg/built",
+    "clean:func": "find built/functional -mindepth 1 \\( ! -path 'built/functional/*.js.snap' ! -path 'built/functional/*.js.md' \\) -delete",


If this gets any longer, move it into scripts/.

Droogans · 2017-11-02T21:02:35Z

test/package.json

    "postinstall": "[ -f bin/visreg.config.ts ] || cp bin/visreg{.example,}.config.ts",
    "previsreg": "npm run clean:visreg",
    "visreg": "npm run build && node built/bin/util.js && node built/bin/visreg.js",
    "webdriver": "npm run webdriver:update && npm run webdriver:start",
    "webdriver:start": "scripts/webdriver-start",
    "webdriver:update": "scripts/webdriver-update"
+  },
+  "ava": {
+    "snapshotLocation": "~/code/js/helix-ui/test/snapshots",


whoops.

I left this in as a failed attempt to get snapshotLocations honored.

This is not a working feature.... 👎

Droogans · 2017-11-02T21:04:05Z

test/visreg/visreg.ts

+        await snap("{browserName}/nav/guides", $(util.selectors.nav));
+    });
+
+    test(`components`, async () => {


Drop the template literals.

CITguy · 2017-11-02T21:07:15Z

Dev LGTM

Droogans · 2017-11-02T21:13:39Z

https://jira.rax.io/browse/SURF-704
https://jira.rax.io/browse/SURF-705

Are now made.

Droogans added 8 commits October 24, 2017 14:36

spike(jest): Setup and switch from mocha/chai

06908a7

refact(visreg): Share one set of tests for both browsers

0af6a1f

fix(visreg): Remove debugging 'exit(0)' statement

66fd923

fix(visreg): Allow for informational messages again

ac2a24c

fix(visreg): Use geckodriver

be4955b

refact(visreg): Move tests out of util

2d19b81

chore(jest): Don't load browser context for visreg

6aa9dc9

test(jest): Outline 'functional' tests

cfad2a8

Droogans requested review from CITguy and cathysiller October 30, 2017 21:48

CITguy suggested changes Oct 31, 2017

View reviewed changes

cathysiller approved these changes Oct 31, 2017

View reviewed changes

Droogans added 3 commits October 31, 2017 16:43

refact(tests): Split each test type into dedicated directory

8cf7382

docs(util): Better document 'page object' and its future

97fb831

style(testing): Avoid scientific notation for big numbers

335bc09

Droogans added 6 commits November 1, 2017 14:48

fix(script): Update reference away from deleted script

274af5b

fix(tsconfig): Include custom type hints

2b9c9f1

fix(visreg): Honor the actual screenshot directory

0ab7eda

fix(visreg): Wrong path to visreg config file

b527452

fix(visreg): Refer to new npm run script

e98239a

fix(jest): Don't spin up jsdom environment

27c2b95

CITguy suggested changes Nov 1, 2017

View reviewed changes

fix(jest): Optimize for selenium

bdc0a8e

fix(visreg): Avoid large png files

3f9b68a

Droogans added 4 commits November 2, 2017 14:58

chore(yarn): Switch to ava

3a7ed64

test(ava): Convert to ava

59af4f8

style(visreg): Remove missing comment

adba06e

style(visreg): Remove unused template literals

47e2666

CITguy reviewed Nov 2, 2017

View reviewed changes

Droogans commented Nov 2, 2017

View reviewed changes

CITguy approved these changes Nov 2, 2017

View reviewed changes

Droogans mentioned this pull request Nov 2, 2017

spike(ava): Switch from mocha/chai #69

Merged

3 tasks

Droogans closed this Nov 2, 2017

CITguy deleted the jest-spike branch March 7, 2018 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jest spike #66

Jest spike #66

Droogans commented Oct 30, 2017 •

edited by CITguy

Loading

CITguy Oct 31, 2017

CITguy Oct 31, 2017

CITguy Oct 31, 2017

CITguy Oct 31, 2017

CITguy Oct 31, 2017

CITguy commented Oct 31, 2017

cathysiller left a comment

Droogans commented Oct 31, 2017

Droogans commented Nov 1, 2017

CITguy Nov 1, 2017

Droogans Nov 1, 2017

Droogans commented Nov 1, 2017

Droogans commented Nov 2, 2017

Droogans commented Nov 2, 2017

CITguy Nov 2, 2017

CITguy Nov 2, 2017

Droogans commented Nov 2, 2017

Droogans Nov 2, 2017

Droogans Nov 2, 2017

Droogans Nov 2, 2017

CITguy commented Nov 2, 2017

Droogans commented Nov 2, 2017

Jest spike #66

Jest spike #66

Conversation

Droogans commented Oct 30, 2017 • edited by CITguy Loading

LGTM's

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

Choose a reason for hiding this comment

CITguy commented Oct 31, 2017

cathysiller left a comment

Choose a reason for hiding this comment

Droogans commented Oct 31, 2017

Droogans commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Droogans commented Nov 1, 2017

Droogans commented Nov 2, 2017

Droogans commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Droogans commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CITguy commented Nov 2, 2017

Droogans commented Nov 2, 2017

Droogans commented Oct 30, 2017 •

edited by CITguy

Loading