Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimelineMarkers snapshot test #353

Merged
merged 3 commits into from
Jun 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
471 changes: 471 additions & 0 deletions flow-typed/npm/jest_v20.x.x.js

Large diffs are not rendered by default.

19 changes: 14 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,12 @@
"browserify": "^13.0.1",
"chai": "^3.5.0",
"css-loader": "^0.26.0",
"devtools-license-check": "^0.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other changes are npm install noise

"eslint": "^3.10.2",
"eslint-config-google": "^0.6.0",
"eslint-plugin-flowtype": "^2.30.0",
"eslint-plugin-import": "^2.2.0",
"eslint-plugin-react": "^6.4.0",
"devtools-license-check": "^0.2.0",
"fake-indexeddb": "^1.0.12",
"file-loader": "^0.9.0",
"flow-bin": "^0.40.0",
Expand All @@ -121,6 +121,7 @@
"lodash.clonedeep": "^4.5.0",
"mkdirp": "^0.5.1",
"raw-loader": "^0.5.1",
"react-test-renderer": "^15.5.4",
"rimraf": "^2.5.4",
"sinon": "^2.1.0",
"style-loader": "^0.13.1",
Expand All @@ -130,10 +131,18 @@
"webpack-hot-middleware": "^2.13.2"
},
"jest": {
"collectCoverageFrom" : ["src/**/*.{js,jsx}", "!**/node_modules/**"],
"testEnvironment": "node",
"moduleFileExtensions": ["js", "jsx"],
"moduleDirectories": ["node_modules"],
"collectCoverageFrom": [
"src/**/*.{js,jsx}",
"!**/node_modules/**"
],
"testEnvironment": "jsdom",
"moduleFileExtensions": [
"js",
"jsx"
],
"moduleDirectories": [
"node_modules"
],
"moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/src/test/fixtures/mocks/file-mock.js",
"\\.(css|less)$": "<rootDir>/src/test/fixtures/mocks/style-mock.js"
Expand Down
28 changes: 18 additions & 10 deletions src/content/components/TimelineCanvas.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@ export default class TimelineCanvas<HoveredItem> extends Component<
_requestedAnimationFrame: boolean;
_devicePixelRatio: 1;
_ctx: CanvasRenderingContext2D;
_canvas: ?HTMLCanvasElement;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


constructor(props: Props<HoveredItem>) {
super(props);
this._requestedAnimationFrame = false;
this._devicePixelRatio = 1;
this.state = { hoveredItem: null };

(this: any)._setCanvasRef = this._setCanvasRef.bind(this);
(this: any)._onMouseMove = this._onMouseMove.bind(this);
(this: any)._onMouseOut = this._onMouseOut.bind(this);
(this: any)._onDoubleClick = this._onDoubleClick.bind(this);
Expand All @@ -55,7 +57,7 @@ export default class TimelineCanvas<HoveredItem> extends Component<
this._requestedAnimationFrame = true;
window.requestAnimationFrame(() => {
this._requestedAnimationFrame = false;
if (this.refs.canvas) {
if (this._canvas) {
timeCode(`${className} render`, () => {
this._prepCanvas();
drawCanvas(this._ctx, this.state.hoveredItem);
Expand All @@ -66,39 +68,41 @@ export default class TimelineCanvas<HoveredItem> extends Component<
}

_prepCanvas() {
const {canvas} = this.refs;
const canvas = this._canvas;
const {containerWidth, containerHeight} = this.props;
const {devicePixelRatio} = window;
const pixelWidth: DevicePixels = containerWidth * devicePixelRatio;
const pixelHeight: DevicePixels = containerHeight * devicePixelRatio;
if (!canvas) {
return;
}
// Satisfy the null check for Flow.
const ctx = this._ctx || canvas.getContext('2d');
if (!this._ctx) {
this._ctx = canvas.getContext('2d');
this._ctx = ctx;
}
if (canvas.width !== pixelWidth || canvas.height !== pixelHeight) {
canvas.width = pixelWidth;
canvas.height = pixelHeight;
canvas.style.width = containerWidth + 'px';
canvas.style.height = containerHeight + 'px';
this._ctx.scale(this._devicePixelRatio, this._devicePixelRatio);
ctx.scale(this._devicePixelRatio, this._devicePixelRatio);
}
if (this._devicePixelRatio !== devicePixelRatio) {
// Make sure and multiply by the inverse of the previous ratio, as the scaling
// operates off of the previous set scale.
const scale = (1 / this._devicePixelRatio) * devicePixelRatio;
this._ctx.scale(scale, scale);
ctx.scale(scale, scale);
this._devicePixelRatio = devicePixelRatio;
}
return this._ctx;
}

_onMouseMove(event: SyntheticMouseEvent) {
const { canvas } = this.refs;
if (!canvas) {
if (!this._canvas) {
return;
}

const rect = canvas.getBoundingClientRect();
const rect = this._canvas.getBoundingClientRect();
const x: CssPixels = event.pageX - rect.left;
const y: CssPixels = event.pageY - rect.top;

Expand Down Expand Up @@ -126,6 +130,10 @@ export default class TimelineCanvas<HoveredItem> extends Component<
return this.props.getHoveredItemInfo(hoveredItem);
}

_setCanvasRef(canvas: HTMLCanvasElement) {
this._canvas = canvas;
}

render() {
const { hoveredItem } = this.state;
this._scheduleDraw();
Expand All @@ -137,7 +145,7 @@ export default class TimelineCanvas<HoveredItem> extends Component<
});

return <canvas className={className}
ref='canvas'
ref={this._setCanvasRef}
onMouseMove={this._onMouseMove}
onMouseOut={this._onMouseOut}
onDoubleClick={this._onDoubleClick}
Expand Down
40 changes: 24 additions & 16 deletions src/content/components/TimelineViewport.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

? new WheelEvent('mouse')
: { DOM_DELTA_LINE: 1, DOM_DELTA_PAGE: 2 };

type Props = {
viewportNeedsUpdate: any,
Expand Down Expand Up @@ -68,10 +70,11 @@ const COLLAPSED_ROW_HEIGHT = 34;
export default function withTimelineViewport<T>(WrappedComponent: ReactClass<T>) {
class TimelineViewport extends PureComponent {

props: Props
shiftScrollId: number
zoomRangeSelectionScheduled: boolean
zoomRangeSelectionScrollDelta: number
props: Props;
shiftScrollId: number;
zoomRangeSelectionScheduled: boolean;
zoomRangeSelectionScrollDelta: number;
_container: ?HTMLElement;

state: {
containerWidth: CssPixels,
Expand Down Expand Up @@ -178,14 +181,16 @@ export default function withTimelineViewport<T>(WrappedComponent: ReactClass<T>)
}

_setSize() {
const rect = this.refs.container.getBoundingClientRect();
if (this.state.containerWidth !== rect.width || this.state.containerHeight !== rect.height) {
this.setState({
containerWidth: rect.width,
containerHeight: rect.height,
containerLeft: rect.left,
viewportBottom: this.state.viewportTop + rect.height,
});
if (this._container) {
const rect = this._container.getBoundingClientRect();
if (this.state.containerWidth !== rect.width || this.state.containerHeight !== rect.height) {
this.setState({
containerWidth: rect.width,
containerHeight: rect.height,
containerLeft: rect.left,
viewportBottom: this.state.viewportTop + rect.height,
});
}
}
}

Expand Down Expand Up @@ -224,13 +229,14 @@ export default function withTimelineViewport<T>(WrappedComponent: ReactClass<T>)

isViewportOccluded(event: SyntheticWheelEvent): boolean {
const scrollElement = this.props.getScrollElement();
if (!scrollElement) {
const container = this._container;
if (!scrollElement || !container) {
return false;
}
// Calculate using getBoundingClientRect to get non-rounded CssPixels.
const innerScrollRect = scrollElement.children[0].getBoundingClientRect();
const scrollRect = scrollElement.getBoundingClientRect();
const viewportRect = this.refs.container.getBoundingClientRect();
const viewportRect = container.getBoundingClientRect();

if (event.deltaY < 0) {
// ______________ viewportRect
Expand Down Expand Up @@ -469,7 +475,9 @@ export default function withTimelineViewport<T>(WrappedComponent: ReactClass<T>)
<div className={viewportClassName}
onWheel={this._mouseWheelListener}
onMouseDown={this._mouseDownListener}
ref='container'>
ref={container => {
this._container = container;
}}>
Copy link
Contributor

@julienw julienw Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<WrappedComponent containerWidth={containerWidth}
containerHeight={containerHeight}
viewportLeft={viewportLeft}
Expand Down
80 changes: 80 additions & 0 deletions src/test/components/TimelineMarkers.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

// @flow
import React from 'react';
import TimelineMarkers from '../../content/containers/TimelineMarkers';
import renderer from 'react-test-renderer';
import { Provider } from 'react-redux';
import mockCanvasContext from '../fixtures/mocks/canvas-context';
import { storeWithProfile } from '../fixtures/stores';
import { getProfileWithMarkers } from '../store/fixtures/profiles';

jest.useFakeTimers();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)


it('renders TimelineMarkers correctly', () => {
// Tie the requestAnimationFrame into jest's fake timers.
window.requestAnimationFrame = fn => setTimeout(fn, 0);
window.devicePixelRatio = 1;
const ctx = mockCanvasContext();

/**
Copy link
Member Author

@gregtatum gregtatum May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Mock out any created refs for the components with relevant information.
*/
function createNodeMock(element) {
// <TimelineCanvas><canvas /></TimelineCanvas>
if (element.type === 'canvas') {
return {
getBoundingClientRect: () => _getBoundingBox(200, 300),
getContext: () => ctx,
style: {},
};
}
// <TimelineViewport />
if (element.props.className.split(' ').includes('timelineViewport')) {
return {
getBoundingClientRect: () => _getBoundingBox(200, 300),
};
}
return null;
}

const profile = getProfileWithMarkers([
['Marker A', 0, {startTime: 0, endTime: 10}],
['Marker B', 0, {startTime: 0, endTime: 10}],
['Marker C', 0, {startTime: 5, endTime: 15}],
]);

const timeline = renderer.create(
<Provider store={storeWithProfile(profile)}>
<TimelineMarkers threadIndex={0} viewHeight={1000} />
</Provider>,
{createNodeMock}
);

// Flush any requestAnimationFrames.
jest.runAllTimers();

const tree = timeline.toJSON();
const drawCalls = ctx.__flushDrawLog();

expect(tree).toMatchSnapshot();
expect(drawCalls).toMatchSnapshot();

delete window.requestAnimationFrame;
delete window.devicePixelRatio;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

function _getBoundingBox(width, height) {
return {
width,
height,
left: 0,
x: 0,
top: 0,
y: 0,
right: width,
bottom: height,
};
}
Loading