From f7e787460271402d95b3dc5cdebcb78ff91f9ef0 Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Tue, 2 May 2017 00:41:47 -0400 Subject: [PATCH] Start a test-report blog post --- docs/updates/5-1-2017-flow.md | 153 +++++++++++++++++++++++++++++++++ docs/updates/5-1-2017-tests.md | 139 ++++++++++++++++++++++++++++++ 2 files changed, 292 insertions(+) create mode 100644 docs/updates/5-1-2017-flow.md create mode 100644 docs/updates/5-1-2017-tests.md diff --git a/docs/updates/5-1-2017-flow.md b/docs/updates/5-1-2017-flow.md new file mode 100644 index 0000000000..d01a00be7c --- /dev/null +++ b/docs/updates/5-1-2017-flow.md @@ -0,0 +1,153 @@ +### Flow Coverage Report (May 1st) + +We're currently using Flow 0.47 and have 60% coverage. Here's a quick run down of how we're doing. + + +#### Components + +* **54%** coverage +* 44 components are typed (almost all) +* `yarn flow-react` - creates a flow report for components + +Typing our components helps us with rendering logic, which can take advantage +of our domain objects such as `Frame`. + +```js +function renderFrameTitle(frame: Frame) { + const displayName = formatDisplayName(frame); + return dom.div({ className: "title" }, displayName); +} +``` + +We are not yet typing our props and state, which would be a helpful way +to get additional coverage. We currently use `PropTypes`, which do not include +flow types. We will hopefully switch to [props] soon to take advantage of static typing. + +```js +shouldComponentUpdate(nextProps, nextState) { + const { frames, selectedFrame } = this.props; + const { showAllFrames } = this.state; + return ( + frames !== nextProps.frames || + selectedFrame !== nextProps.selectedFrame || + showAllFrames !== nextState.showAllFrames + ); +} +``` + +There have been a couple places where flow types have been difficult to get right. + +1. **covariants** `this.toggleFramesDisplay = this.toggleFramesDisplay.bind(this)`. We have an [issue](https://github.com/devtools-html/debugger.html/issues/3172) to improve this. +2. **defaultProps** and other class properties have been awkward to add types for. + +Overall, typing our components has been a nice win though as it gives us additional confidence our UI will behave appropriately and catches several lifecycle edge-cases +where properties could be null or racey. + +#### Utilities + +* **59%** coverage +* **60** files +* `yarn flow-utils` - creates a flow report for utils + +We started typing our utility functions. Starting with utilities helped us +document our primary business logic and made them easier to refactor. + +In this example, we added types to a parser utility that checks to see if an +expression is in a particular scope. + +```js +export function isExpressionInScope(expression: string, scope?: Scope) { + if (!scope) { + return false; + } + + const variables = getVariablesInScope(scope); + const firstPart = expression.split(/\./)[0]; + return variables.includes(firstPart); +} +``` + +We are not currently typing our test files, but I think it would be helpful if +we did. In this example, we are searching for all of the functions in a given +source, but because we did not provide a type for `getSourceText`, flow can +not use `getSymbols`'s types to tell us if we're writing a reasonable test. + +```js +it("finds functions", () => { + const fncs = getSymbols(getSourceText("func")).functions; + const names = fncs.map(f => f.value); + expect(names).to.eql(["square", "child", "anonymous"]); +}); +``` + +Overall, typing our utilities was a great first step. And the more that we type +the components, actions, and reducers that call the utilities, the more value +we will get out of them. Already, the types are forcing an internal consistency +that helps us achieve a better API for our utils. + +#### Actions / Reducers + +Our Actions and Reducers are generally very well covered. + +* **78%** coverage +* **19** files +* `yarn flow-redux` - creates a flow report for actions and reducers + +Actions like breakpoints and pause have a coverage score of over 80%. +We initially, typed our action dispatcher params. We've recently added +types for `ThunkArgs` and `State`. Typing `State`, allows flow to type check +our selectors and utils. + + +```js +export function newSource(source: Source) { + return ({ dispatch, getState }: ThunkArgs) => { + if (prefs.clientSourceMapsEnabled) { + dispatch(loadSourceMap(source)); + } + + dispatch({ type: constants.ADD_SOURCE, source }); + + checkSelectedSource(getState(), dispatch, source); + checkPendingBreakpoints(getState(), dispatch, source); + }; +} +``` + +Our reducers are 80% covered and some reducers like breakpoints and pause are 80+ +percent covered. Breakpoints is actually 97% covered. Most of the coverage comes +from our State type and Action union types. + +The `State` type describes the state of the the reducer store. Typing the State +and action types lets us fully describe our reducers and get a high flow coverage +score. + +```js +export type BreakpointsState = { + breakpoints: I.Map, + pendingBreakpoints: any[], + breakpointsDisabled: false +}; +``` + +Typing our actions gives us confidence that the action dispatched will be +what our reducers expect to handle. + +```js +type BreakpointAction = + | { type: "ADD_BREAKPOINT", + breakpoint: Breakpoint, + condition: string, + value: BreakpointResult + } + | { + type: "REMOVE_BREAKPOINT", + breakpoint: Breakpoint, + disabled: boolean + } +... +export type Action = SourceAction | BreakpointAction | PauseAction | UIAction; +``` + + +[props]: https://flow.org/en/docs/frameworks/react/#toc-adding-types-for-react-component-props diff --git a/docs/updates/5-1-2017-tests.md b/docs/updates/5-1-2017-tests.md new file mode 100644 index 0000000000..1b16bb0c07 --- /dev/null +++ b/docs/updates/5-1-2017-tests.md @@ -0,0 +1,139 @@ +## Test Coverage Report (May 1st) + +### Unit Tests - Jest + +#### Components + +* **6** files +* **80+** coverage +* **60** files of coverage + +We use Jest to test our React Component. We mostly do snapshot testing to +check for changes over time. We also have interaction tests where we test to see +if user input like a "click" will fire the appropriate handler. + +Jest's default react shallow renderer with enzyme works well for us because it +limits the scope of the test and lets us focus on the component under testing and +not necessarily the childen. We also test wrapped components to avoid the connected function and passing in the full application state. + +With that said, a typical component test will still exercise several the utilities +and other imported dependencies. We just started writing component tests and our 6 tests +cover 60 files! We're not trying to write isolated unit tests! + + +##### Snapshot Test + +```js +it("should render the component", () => { + const wrapper = shallow(ResultList(payload)); + expect(wrapper).toMatchSnapshot(); +}); +``` + +##### Interaction Test + +```js +it("should call onClick function", () => { + const wrapper = shallow(ResultList(payload)); + + wrapper.childAt(selectedIndex).simulate("click"); + expect(selectItem).toBeCalled(); +}); +``` + +#### Utilities + +Our utilities are the best tested part of the debugger. We try to move as much +logic to a util so that we can reasonably test them in isolation. + +The best example of this is our parser utils, which are like the brain of the +debugger. Here's a really cool example of how we test getting local variables. +The best part about these tests is that they're really easy to TDD and get +confidence. It lets us have 90-100% coverage and refactor really easily. + +```js +it("only gets local variables", () => { + const scope = getClosestScope(getSourceText("math"), { + line: 3, + column: 5 + }); + + var vars = getVariablesInLocalScope(scope); + + expect(vars.map(v => v.name)).to.eql(["n"]); + expect(vars[0].references[0].node.loc.start).to.eql({ + column: 4, + line: 3 + }); +}); +``` + +#### Actions / Reducers + +* **4** files and 37 tests +* **80%** coverage +* **50** files of coverage + +The old Debugger UI only had integration tests. When we switched to React, we +knew we wanted to write tests that covered our Redux system. + +The first we wrote tested our breakpoint logic. I still think it reads really +well and covers a lot of ground. Notice that we have a mock client that we pass +into the store so that we don't make any API calls. + +```js +it("should add a breakpoint", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + + await dispatch(actions.addBreakpoint({ sourceId: "a", line: 5 })); + await dispatch(actions.addBreakpoint({ sourceId: "b", line: 6 })); + + expect(selectors.getBreakpoints(getState()).size).to.be(2); +}); +``` + +We've recently made a concerted effort to add more redux integration test. +We're now testing all of our action files and have good coverage in everything, +but source actions. + +--- + +### Integration Tests + +While we try to get most of our coverage with unit tests, it is still helpful +to have integration tests to confirm that we did not break an important code path +like pausing on a breakpoint. + +Switching to redux has helped us write really clean tests, +that are both fun to write, and easy to debug. + +Here is a typical test where we open the debugger, toggle breakpoints by clicking in the gutter, and check the redux store if the beakpoint is registered. + +```js +async function toggle(ctx) { + const { ok, is, info } = ctx; + const dbg = await initDebugger("doc-scripts.html", "simple2"); + + // Create two breakpoints + await selectSource(dbg, "simple2"); + await addBreakpoint(dbg, "simple2", 3); + await addBreakpoint(dbg, "simple2", 5); + + // Disable the first one + await disableBreakpoint(dbg, 1); + let bp1 = findBreakpoint(dbg, "simple2", 3); + let bp2 = findBreakpoint(dbg, "simple2", 5); + is(bp1.disabled, true, "first breakpoint is disabled"); + is(bp2.disabled, false, "second breakpoint is enabled"); + + // Disable and Re-Enable the second one + await disableBreakpoint(dbg, 2); + await enableBreakpoint(dbg, 2); + bp2 = findBreakpoint(dbg, "simple2", 5); + is(bp2.disabled, false, "second breakpoint is enabled"); +} +``` + +Our integration tests are our last line of defense and +have caught the most would be bugs. We are currently making a push to test +all of our UI components with at least one integration test.