tests: move cli/core to jest #5386

patrickhulce · 2018-05-30T21:40:14Z

in pursuit of #5383

Before	After

only real code changes are before -> beforeAll, after -> afterAll, and moving .timeout

lots of noise from eslint-env mocha though :/

it also caught a bug that's not clear at all how the mocha tests were passing in CLI case where requiring in the yargs getFlags invokes process.exit (!!!!!). there's another psuedobug where we pass in 'Unknown' strings to our parseURL function which can throw

I'm not using jest's built-in expectations/coverage/anything yet to keep the diff small and easily revertable if folks don't like it.

paulirish

What happens to coverage? (Apparently jest can collect on its own. But regardless we should probably do that later)

I don't think --watch works correctly on our project. It doesn't associate related files, it seems.

paulirish · 2018-05-30T21:48:34Z

jest.config.js

+    '**/lighthouse-core/**/*-test.js',
+    '**/lighthouse-cli/**/*-test.js',
+  ],
+};


let's set the timeout to 2000ms, since that's what we use in mocha. (otherwise it'll default to 5000ms)

how important is this one? do we think 2s was a good feature? :)

in jest you do this via jest.setTimeout which requires a bootstrap file, and AFAICT cannot be done through config https://facebook.github.io/jest/docs/en/configuration.html

oic. hmmmmmmmm well nevermind then.

paulirish · 2018-05-30T21:51:23Z

jest.config.js

@@ -0,0 +1,7 @@
+module.exports = {


gotta disable eslint

we could go for /* eslint-env jest */

ya @wardpeet I made that change 👍 :)

paulirish · 2018-05-30T21:52:19Z

lighthouse-core/test/lib/asset-saver-test.js

@@ -27,7 +27,7 @@ function assertTraceEventsEqual(traceEventsA, traceEventsB) {
 /* eslint-env mocha */


gotta replace all these with eslint-env jeset

paulirish · 2018-05-30T21:55:04Z

package.json

-
-    "unit-core": "mocha --reporter dot \"lighthouse-core/test/**/*-test.js\"",
-    "unit-cli": "mocha --reporter dot \"lighthouse-cli/test/**/*-test.js\"",
+    "unit-core": "jest \"lighthouse-core/\"",


looks like we want to runInBand on CI?

vs

yeah not sure how we should do this one, since we reuse lots of commands, suggestions?

I just went with a :ci copy of unit-core 👍

paulirish · 2018-05-30T22:55:47Z

Also this is mad cool. Excited for this. :D

paulirish · 2018-05-30T23:20:18Z

coverage

for whatever reason this appears to drop our coverage % down quite a bit. not sure what's happening, though. https://codecov.io/gh/GoogleChrome/lighthouse/pull/5386

patrickhulce · 2018-05-30T23:23:11Z

not sure what's up with the coverage freaking out 🤔

patrickhulce · 2018-05-31T16:37:16Z

OK I sorted out the coverage problem thanks to jestjs/jest#3190 (comment) 😃 🎉

brendankenny

I wasn't there for the conversation, so what's the advantage here in changing?

It's a pretty transparent change so the code side doesn't seem to matter much, but it does seem significantly slower in CI, at least

brendankenny · 2018-05-31T17:05:51Z

lighthouse-core/report/html/renderer/util.js

-      hostname: parsedUrl.hostname,
-      origin: parsedUrl.origin,
-    };
+    try {


why was this needed?

well we only need this or a fix on the _renderTextURL to be looser on its try/catch (we check instanceof TypeError), but it seemed like a good catch on passing invalid URLs to parseURL. We pass through several 'Unknown' strings to this function which throw

hmm, but why was it needed for this PR? If it's throwing shouldn't it have caused a failure before, too? Or is the problem something else.

well again it isn't strictly needed, but seemed like a good idea anyhow.

full details:
in jest's jsdom env, the URL class throws an error that isn't instanceof TypeError
_renderTextURL which normally catches TypeErrors thrown by this method was then throwing

this isn't needed anymore because along the way I turned off default jsdom environment, but it seemed like a good idea to keep, it seems like you don't think so though so I'll revert :)

brendankenny · 2018-05-31T17:06:46Z

lighthouse-core/test/gather/computed/manifest-values-test.js

@@ -28,7 +28,7 @@ function noUrlManifestParser(manifestSrc) {
  return manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
 }

-/* eslint-env mocha */
+/* eslint-env jest */


delete this one?

brendankenny · 2018-05-31T17:16:10Z

package.json

-    "coverage": "nyc yarn unit && nyc report --reporter html",
-    "smoke:silentcoverage": "nyc --silent yarn smoke && nyc report --reporter text-lcov > smoke-coverage.lcov",
-    "coverage:smoke": "nyc yarn smoke && nyc report --reporter html",
+    "unit:silentcoverage": "nyc --silent --clean yarn unit:ci && nyc report --reporter text-lcov > unit-coverage.lcov",


one unfortunate side of this (at least judging from that comment in the jest issue) is that subprocesses don't automatically collect coverage. I don't believe we do that in any unit tests, but it is a handy thing to lose

since jest automatically collects coverage with --coverage, are the lcov files written anywhere so we drop the nyc calls (for non-smoke tests) altogether?

if we use jest everywhere we won't need nyc for the unit tests at all and we can use the lcov/html files they generate in ./coverage

right now, jest coverage output is just disabled and it lets nyc collect/combine everything since we still need it for the mocha cases

brendankenny · 2018-05-31T17:19:33Z

package.json

+    "unit:silentcoverage": "nyc --silent --clean yarn unit:ci && nyc report --reporter text-lcov > unit-coverage.lcov",
+    "coverage": "nyc --clean yarn unit:ci && nyc report --reporter html",
+    "smoke:silentcoverage": "nyc --silent --clean yarn smoke && nyc report --reporter text-lcov > smoke-coverage.lcov",
+    "coverage:smoke": "nyc --clean yarn smoke && nyc report --reporter html",


FWIW I believe --clean is the default

patrickhulce · 2018-06-15T19:01:43Z

so what's the story here, are we sold on jest? sticking with mocha? undecided? :)

wardpeet · 2018-06-25T21:44:17Z

@patrickhulce I'm not hearing any people saying no that means we can go for Jest, right? 😛

patrickhulce · 2018-06-25T21:47:19Z

I like the way you think @wardpeet, I'm thinking auto-merge in 48 hours if no more objections? ;)

wardpeet · 2018-06-25T21:48:18Z

They might not even notice 😄

paulirish · 2018-06-26T00:12:46Z

The advantages that I'm jazzed about:

Way faster locally.
Display is very concise. You can see failing tests immediately, but passing tests don't scroll the view.
Update expectations snapshot with a keystroke sounds soooooo good.

The disadvantages that I'm bummed about:

A bit slower on travis.

On the whole though i think it'll be an improved experience.

patrickhulce · 2018-06-26T20:24:33Z

Alright merge conflicts fixed! @paulirish do you still have requested changes?

paulirish · 2018-06-26T20:51:29Z

package.json

@@ -93,6 +96,7 @@
    "gulp": "^3.9.1",
    "gulp-replace": "^0.5.4",
    "gulp-util": "^3.0.7",
+    "jest": "^23.0.1",


can now use 23.2.0

brendankenny

I still don't see what's "mad cool" about this, but I guess we'll see eventually? :)

Ideally we can get rid of nyc and mocha in the future so we don't have to maintain all three.

brendankenny · 2018-06-27T21:56:25Z

lighthouse-core/report/html/renderer/util.js

-      hostname: parsedUrl.hostname,
-      origin: parsedUrl.origin,
-    };
+    try {


hmm, but why was it needed for this PR? If it's throwing shouldn't it have caused a failure before, too? Or is the problem something else.

tests: move core to jest

2214c12

patrickhulce requested review from paulirish and vinamratasingal-zz as code owners May 30, 2018 21:40

paulirish deployed to PR staging May 30, 2018 21:40 View deployment

patrickhulce changed the title ~~tests: move core to jest~~ tests: move cli/core to jest May 30, 2018

paulirish requested changes May 30, 2018

View reviewed changes

feedback

c4309a7

patrickhulce requested review from brendankenny and ebidel as code owners May 30, 2018 23:08

paulirish deployed to PR staging May 30, 2018 23:08 View deployment

patrickhulce added 4 commits May 30, 2018 16:53

coverage testing

40a762c

fix cli flags test

d1339b3

cli flags wtf

cd618db

add url to getFlags call

43337b3

brendankenny reviewed May 31, 2018

View reviewed changes

feedback

45b60bc

patrickhulce added 2 commits June 26, 2018 11:01

Merge branch 'master' into jest

069e272

Merge branch 'master' into jest

8b6b695

paulirish approved these changes Jun 26, 2018

View reviewed changes

paulirish reviewed Jun 26, 2018

View reviewed changes

jest 23.2

efea9df

brendankenny approved these changes Jun 27, 2018

View reviewed changes

patrickhulce and others added 3 commits June 28, 2018 09:44

no more try/catch

db79786

Merge branch 'master' into jest

1c51e6e

main thread tasks -> jest

373b161

paulirish merged commit aa43403 into master Jun 29, 2018

paulirish deleted the jest branch June 29, 2018 01:45

paulirish mentioned this pull request Nov 2, 2018

fix(report): edge Invalid argument error #6456

Merged

		@@ -27,7 +27,7 @@ function assertTraceEventsEqual(traceEventsA, traceEventsB) {
		/* eslint-env mocha */

tests: move cli/core to jest #5386

tests: move cli/core to jest #5386

Conversation

patrickhulce commented May 30, 2018 • edited Loading

paulirish 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Jun 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 30, 2018

paulirish commented May 30, 2018

patrickhulce commented May 30, 2018

patrickhulce commented May 31, 2018

brendankenny 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jun 15, 2018

wardpeet commented Jun 25, 2018

patrickhulce commented Jun 25, 2018

wardpeet commented Jun 25, 2018

paulirish commented Jun 26, 2018

patrickhulce commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented May 30, 2018 •

edited

Loading

wardpeet Jun 3, 2018 •

edited

Loading