Upgrade to jest 22 #832

julienw · 2018-03-06T17:47:39Z

No description provided.

julienw · 2018-03-06T17:48:40Z

.babelrc

    [
-      "transform-class-properties"
+      "transform-builtin-extend",


This is needed so that "XXX instanceof SubclassedError" works... Once we stop transpiling classes we can also remove this...

julienw · 2018-03-06T17:49:09Z

src/actions/receive-profile.js

@@ -83,6 +83,9 @@ let requestIdleCallbackPolyfill: (

 if (typeof window === 'object' && window.requestIdleCallback) {
  requestIdleCallbackPolyfill = window.requestIdleCallback;
+} else if (typeof process === 'object' && process.nextTick) {
+  // Node environment
+  requestIdleCallbackPolyfill = process.nextTick;


This is done so that we can easily use fake timers and still have requestIdleCallback works

julienw · 2018-03-06T17:49:38Z

src/profile-logic/symbol-store-db.js

@@ -38,7 +40,7 @@ const kTwoWeeksInMilliseconds = 2 * 7 * 24 * 60 * 60 * 1000;
 * @class SymbolStoreDB
 * @classdesc Where does this description show up?
 */
-export class SymbolStoreDB {
+export default class SymbolStoreDB {


This is done so that I could mock it with jest... I couldn't find how to do with a named export :/

julienw · 2018-03-06T17:50:14Z

src/test/components/__snapshots__/FlameGraph.test.js.snap

@@ -69,7 +69,7 @@ exports[`renders FlameGraph correctly 1`] = `
    <div
      className="flameGraphLabels grippy"
      title="thread: \\"Empty\\" (0)
-    process: \\"default\\" (0)"
+process: \\"default\\" (0)"


Looks like they changed how they generate snapshot for multiline string attributes

julienw · 2018-03-06T17:50:32Z

src/test/components/__snapshots__/MarkerChart.test.js.snap

@@ -337,7 +337,18 @@ Array [
  Array [
    "set fillStyle",
    Object {
-      "addColorStop": [Function],
+      "addColorStop": [MockFunction] {


Now Jest can serialize mock functions' calls !

julienw · 2018-03-06T17:54:21Z

src/test/store/receive-profile.test.js

+        getSymbolTable: jest.fn().mockResolvedValue(exampleSymbolTable),
+      }));
+
+      window.TextDecoder = TextDecoder;


Various issues here, coming from the fact that we don't have indexedDB in this test file, and so SymbolStoreDB was broken. The errors weren't transmitted back to the test, but because in SymbolStore._getSymbolTable we're storing stuff in the database but not waiting for it in the promise, the errors were "leaking" in other tests.

So I took the path of:

Throwing back unexpected errors to the top level calls, failing the right tests

Fixing the tests by mocking SymbolStoreDB itself, making it returning a proper value so that we don't try to store data, etc. We don't need this in this test.

This proved quite a lot of work :)

julienw · 2018-03-06T17:56:20Z

src/types/libdef/npm/jest_v22.x.x.js

+  ): JestMockFn<TArguments, TReturn>,
+  mockResolvedValueOnce(
+    value: ExtractPromiseValue<TReturn>
+  ): JestMockFn<TArguments, TReturn>,


This is the important stuff.
This is not working as expected really, but enough for this PR I think. I fired a PR at flow-typed: flow-typed/flow-typed#1918, see comments there

julienw · 2018-03-06T17:56:44Z

src/types/libdef/npm/jest_v22.x.x.js

-// flow-typed signature: d5b32a3045854623325a334220fbc654
-// flow-typed version: c7c67b81c1/jest_v20.x.x/flow_>=v0.39.x
+// flow-typed signature: 18018da6c1a1d95b4ab1c64bb5fe86ca
+// flow-typed version: c1ad61e7d4/jest_v22.x.x/flow_>=v0.39.x


Too much changes in this file because of prettier, I'll revert most of the changes before landing...

codecov · 2018-03-06T17:58:29Z

Codecov Report

Merging #832 into master will increase coverage by 0.46%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
+ Coverage   66.92%   67.38%   +0.46%     
==========================================
  Files         127      128       +1     
  Lines        7344     5817    -1527     
  Branches     1670      884     -786     
==========================================
- Hits         4915     3920     -995     
+ Misses       2133     1632     -501     
+ Partials      296      265      -31

Impacted Files	Coverage Δ
src/profile-logic/symbol-store-db.js	`87.23% <ø> (-0.84%)`	⬇️
src/profile-logic/symbolication.js	`91.76% <0%> (-3.09%)`	⬇️
src/profile-logic/symbol-store-db-worker.js	`0% <0%> (ø)`	⬆️
src/profile-logic/errors.js	`100% <100%> (ø)`
src/utils/errors.js	`100% <100%> (ø)`	⬆️
src/actions/receive-profile.js	`63.46% <100%> (-9.13%)`	⬇️
src/profile-logic/symbol-store.js	`80% <33.33%> (-0.49%)`	⬇️
src/utils/flow.js	`56.25% <0%> (-15.98%)`	⬇️
src/utils/dom-rect.js	`0% <0%> (-10%)`	⬇️
... and 115 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31228a1...5ddd883. Read the comment docs.

mstange

Thanks for the explanations. I don't know very much about all this, but what you say makes sense and the changes look non-controversial.

jasonLaster · 2018-03-06T19:50:48Z

Nice @julienw

mstange · 2018-03-06T20:33:44Z

There are some test failures.

julienw · 2018-03-07T10:25:38Z

This is weird, these test failures look like something I fixed...

julienw · 2018-03-07T10:55:32Z

Ah, this is fun, the error happens only with --coverage.

…at these files

Especially this introduces a specialized Error for symbolication, so that we can distinguish errors and more easily debug tests by rethrowing unexpected errors.

julienw · 2018-03-07T11:29:48Z

My theory is that when collecting coverage some more instrumentation is added to the files and as a result the babel transform doesn't work anymore.

I removed the transforms, and instead I manually added a line to the constructor:

   constructor(message: string, library: Library, error?: Error) {
     super(message);
    // Workaround for a babel issue when extending Errors
    (this: any).__proto__ = SymbolsNotFoundError.prototype;
     this.name = 'SymbolsNotFoundError';
     this.library = library;
     this.error = error;
   }

Other alternatives:

compare the property name instead of using instanceof
do not subclass Error but just call the Error.call(this, message) in the constructor (we should check if this captures the stack properly)

julienw commented Mar 6, 2018

View reviewed changes

julienw requested a review from mstange March 6, 2018 17:58

mstange approved these changes Mar 6, 2018

View reviewed changes

julienw added 3 commits March 7, 2018 11:26

Upgrade jest to v22

ea76fd1

Upgrade snapshots

55f9155

Upgrade flow typing for newer jest

8bdb24b

julienw force-pushed the upgrade-jest-22 branch from 84ce531 to 37d7dad Compare March 7, 2018 10:26

julienw added 3 commits March 7, 2018 12:09

Avoid collecting coverage from libdefs as the coverage library fails …

eee56d1

…at these files

Fix test errors

ba5e947

Especially this introduces a specialized Error for symbolication, so that we can distinguish errors and more easily debug tests by rethrowing unexpected errors.

Fix instanceof error

5ddd883

julienw force-pushed the upgrade-jest-22 branch from 37d7dad to 5ddd883 Compare March 7, 2018 11:26

julienw merged commit 0161ae4 into firefox-devtools:master Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to jest 22 #832

Upgrade to jest 22 #832

julienw commented Mar 6, 2018

julienw Mar 6, 2018

julienw Mar 6, 2018

julienw Mar 6, 2018

julienw Mar 6, 2018

julienw Mar 6, 2018

julienw Mar 6, 2018

julienw Mar 6, 2018 •

edited

julienw Mar 6, 2018

codecov bot commented Mar 6, 2018 •

edited

mstange left a comment

jasonLaster commented Mar 6, 2018

mstange commented Mar 6, 2018

julienw commented Mar 7, 2018

julienw commented Mar 7, 2018

julienw commented Mar 7, 2018 •

edited

Upgrade to jest 22 #832

Upgrade to jest 22 #832

Conversation

julienw commented Mar 6, 2018

julienw Mar 6, 2018

Choose a reason for hiding this comment

julienw Mar 6, 2018

Choose a reason for hiding this comment

julienw Mar 6, 2018

Choose a reason for hiding this comment

julienw Mar 6, 2018

Choose a reason for hiding this comment

julienw Mar 6, 2018

Choose a reason for hiding this comment

julienw Mar 6, 2018

Choose a reason for hiding this comment

julienw Mar 6, 2018 • edited

Choose a reason for hiding this comment

julienw Mar 6, 2018

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2018 • edited

Codecov Report

mstange left a comment

Choose a reason for hiding this comment

jasonLaster commented Mar 6, 2018

mstange commented Mar 6, 2018

julienw commented Mar 7, 2018

julienw commented Mar 7, 2018

julienw commented Mar 7, 2018 • edited

julienw Mar 6, 2018 •

edited

codecov bot commented Mar 6, 2018 •

edited

julienw commented Mar 7, 2018 •

edited