TimelineMarkers snapshot test #353

gregtatum · 2017-05-24T15:13:24Z

This is my first attempt at a snapshot test for a component. My thoughts here are that snapshots should be a good first line of defense for catching component regressions. Snapshot tests would only be sanity checks for when things change. They should be fairly lightweight in the implementation, and easy to regenerate. As the components change over time you can see that the snapshots fail, inspect the output, re-generate the snapshot, and then move on with your life. I think these are good candidates for higher level connected components that have some complexity. This tests the integration of several components.

For more a unit-test level of individual components, I think enzyme would be a better fit. You can command the component at a more granular level, and check individual properties. I think the moment that we start trying to do things with the test, we should consider switching over to an enzyme test and be more strict with testing the desired output.

I would say at this point, let's create some enzyme tests of some components, and compare how this looks to a snapshot test. I think a good approach would be to go ahead and merge in these experiments, but with the option to strip them out, or refactor them.

gregtatum · 2017-05-24T15:14:07Z

package.json

@@ -102,12 +102,12 @@
    "browserify": "^13.0.1",
    "chai": "^3.5.0",
    "css-loader": "^0.26.0",
+    "devtools-license-check": "^0.2.0",


This and other changes are npm install noise

gregtatum · 2017-05-24T15:14:54Z

src/content/components/TimelineCanvas.js

@@ -25,6 +25,7 @@ export default class TimelineCanvas extends Component {
  _requestedAnimationFrame: boolean;
  _devicePixelRatio: 1;
  _ctx: CanvasRenderingContext2D;
+  _canvas: ?HTMLCanvasElement;


I had to change the ref implementation to satisfy react-test-renderer

codecov-io · 2017-05-24T15:19:04Z

Codecov Report

Merging #353 into master will increase coverage by 4.38%.
The diff coverage is 79.54%.

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
+ Coverage   31.64%   36.02%   +4.38%     
==========================================
  Files         124      123       -1     
  Lines        5947     5940       -7     
  Branches     1233     1231       -2     
==========================================
+ Hits         1882     2140     +258     
+ Misses       3484     3252     -232     
+ Partials      581      548      -33

Impacted Files	Coverage Δ
src/content/components/TimelineViewport.js	`24.74% <66.66%> (+24.74%)`	⬆️
src/content/components/TimelineCanvas.js	`57.95% <69.23%> (+57.95%)`	⬆️
src/test/fixtures/mocks/canvas-context.js	`94.73% <94.73%> (ø)`
src/content/actions/app.js	`0% <0%> (ø)`	⬆️
src/test/fixtures/mocks/style-mock.js
src/content/actions/index.js
src/content/reducers/timeline-view.js	`93.33% <0%> (+3.33%)`	⬆️
src/content/profile-data.js	`65.42% <0%> (+3.5%)`	⬆️
src/content/process-profile.js	`62.19% <0%> (+3.88%)`	⬆️
... and 6 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 8035392...e58b49f. Read the comment docs.

gregtatum · 2017-05-24T15:19:45Z

src/content/components/TimelineViewport.js

@@ -17,7 +17,9 @@ import type {
 import type { UpdateProfileSelection } from '../actions/profile-view';
 import type { ProfileSelection } from '../actions/types';

-const { DOM_DELTA_PAGE, DOM_DELTA_LINE } = new WheelEvent('mouse');
+const { DOM_DELTA_PAGE, DOM_DELTA_LINE } = (typeof window === 'object' && window.WheelEvent)


Hmm... this was the easiest way to handle this for the test to run, otherwise I'll need to put this information into the component.

or you can add the WheelEvent component in window ? (or file a bug at https://github.com/tmpvar/jsdom ? or even better, do a PR there ;) )

Please at least file a bug there

The problem is that this runs anytime the document is imported, often before I get a chance to be able to inject something into the window, and I have to catch it for every single test that happens to touch this file.

oh I see ! so we can't add it ourselves but still jsdom should have it :)

gregtatum · 2017-05-24T15:21:15Z

src/test/components/TimelineMarkers.test.js

+  window.devicePixelRatio = 1;
+  const ctx = mockCanvasContext();
+
+  /**


createNodeMock is definitely the most brittle part of the test. Not super happy about mocking things out like this, but it lets us use refs.

I think we can do better by mocking (with sinon or jest) stuff in HTMLCanvasElement. But that will do for now.

ah, I see, it's because of facebook/react#7740 :/

Yes, that's the reasoning for switching it up.

gregtatum · 2017-06-05T17:59:12Z

@mstange This is starting to block other component testing, do you think you could take a look at it?

mstange

Looks good

mstange · 2017-06-06T00:13:22Z

src/content/components/TimelineCanvas.js

-                   ref='canvas'
+                   ref={canvas => {
+                     this._canvas = canvas;
+                   }}


Do you know if the fact that you're passing a lambda here is going to cause unnecessary re-renders? In other places we've used this._takeCanvasRef to store the function.
Though it's just really one element, so it probably won't matter.

~~what's the advantage of using this instead of this.refs.canvas ?~~
Ok I read the string refs are now deprecated.

I'm going to fix that before merging. We've been bitten too much in the past with similar types of issues.

julienw · 2017-06-06T10:17:22Z

src/test/store/icons.test.js

  });

  afterEach(() => {
-    delete global.Image;
+    delete window.Image;


this will likely bite us later -- but please proceed, I'll have a look later.

julienw · 2017-06-06T10:21:21Z

src/test/components/TimelineMarkers.test.js

+  expect(drawCalls).toMatchSnapshot();
+
+  delete window.requestAnimationFrame;
+  delete window.devicePixelRatio;


pretty sure this will bite us later :/ I'll have a look at how to make this better

julienw · 2017-06-06T10:21:26Z

src/test/components/TimelineMarkers.test.js

+import { storeWithProfile } from '../fixtures/stores';
+import { getProfileWithMarkers } from '../store/fixtures/profiles';
+
+jest.useFakeTimers();


I don't really like that we call that here at the top level, I'd rather call it in beforeEach so that we can call useRealTimers in an afterEach.

That said I don't really understand why we use fake timers -- don't you need real timers to mock requestAnimationFrame ?

The timers are only fake for the scope of that file, and don't affect other ones. I think I'd prefer not to clutter up the test with extra beforeEach and afterEach declarations.

he requestAnimationFrame mock uses the setTimeout which is faked by jest. I added a few comments in the file explaining the timers and how they are running requestAnimationFrames to clarify.

The timers are only fake for the scope of that file,

Oh OK. Jest is different to mocha in this regard. Good!

about my comment about the fake timers, I missed the call to jest.runAllTimers();, so all is good :)

julienw · 2017-06-06T10:22:02Z

src/content/components/TimelineViewport.js

-             ref='container'>
+             ref={container => {
+               this._container = container;
+             }}>


~~why don't you like using this.refs ?~~ because this is deprecated

gregtatum requested review from julienw and mstange May 24, 2017 15:13

gregtatum commented May 24, 2017

View reviewed changes

gregtatum mentioned this pull request Jun 5, 2017

Tooltips using React DOM #364

Merged

mstange approved these changes Jun 6, 2017

View reviewed changes

julienw reviewed Jun 6, 2017

View reviewed changes

gregtatum force-pushed the timeline-markers-test branch from 51d0674 to 73d9b9b Compare June 6, 2017 20:27

gregtatum added 3 commits June 7, 2017 13:57

Switch jest's env to jsdom

7664708

Add jest libdef

ba6bad2

Add first component test with TimelineMarkers

e58b49f

gregtatum force-pushed the timeline-markers-test branch from 934169b to e58b49f Compare June 7, 2017 19:05

gregtatum merged commit 58e92b2 into firefox-devtools:master Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TimelineMarkers snapshot test #353

TimelineMarkers snapshot test #353

gregtatum commented May 24, 2017

gregtatum May 24, 2017

gregtatum May 24, 2017

codecov-io commented May 24, 2017 •

edited

Loading

gregtatum May 24, 2017

julienw Jun 6, 2017

gregtatum Jun 6, 2017

julienw Jun 7, 2017

gregtatum May 24, 2017 •

edited

Loading

julienw Jun 6, 2017

julienw Jun 6, 2017

gregtatum Jun 6, 2017

gregtatum commented Jun 5, 2017

mstange left a comment

mstange Jun 6, 2017

julienw Jun 6, 2017 •

edited

Loading

gregtatum Jun 6, 2017

julienw Jun 6, 2017

julienw Jun 6, 2017

julienw Jun 6, 2017

gregtatum Jun 6, 2017

julienw Jun 7, 2017

julienw Jun 6, 2017 •

edited

Loading

TimelineMarkers snapshot test #353

TimelineMarkers snapshot test #353

Conversation

gregtatum commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 24, 2017 • edited Loading

Codecov Report

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

gregtatum May 24, 2017 • 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

gregtatum commented Jun 5, 2017

mstange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw Jun 6, 2017 • 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

julienw Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

codecov-io commented May 24, 2017 •

edited

Loading

gregtatum May 24, 2017 •

edited

Loading

julienw Jun 6, 2017 •

edited

Loading

julienw Jun 6, 2017 •

edited

Loading