From 5cab08f27f3c203d698b5629f2738b137d7182da Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Tue, 9 May 2017 16:18:12 +0100 Subject: [PATCH 01/11] Refactor Pending Breakpoints to fix reloading --- src/actions/breakpoints.js | 3 +- src/actions/sources.js | 69 ++++--- src/actions/tests/breakpoints.js | 62 +++++- src/actions/tests/pending-breakpoints.js | 248 +++++++++++++++++++++++ src/actions/tests/sources.js | 3 + src/client/firefox/commands.js | 2 + src/reducers/breakpoints.js | 161 ++++++++------- src/test/tests-setup.js | 1 - 8 files changed, 447 insertions(+), 102 deletions(-) create mode 100644 src/actions/tests/pending-breakpoints.js diff --git a/src/actions/breakpoints.js b/src/actions/breakpoints.js index 1a35b93080..f020542816 100644 --- a/src/actions/breakpoints.js +++ b/src/actions/breakpoints.js @@ -83,7 +83,6 @@ export function addBreakpoint( if (!text) { text = getTextForLine ? getTextForLine(actualLocation.line) : ""; } - return { id, actualLocation, text, hitCount }; })() }); @@ -110,7 +109,7 @@ export function removeBreakpoint(location: Location) { return _removeOrDisableBreakpoint(location); } -function _removeOrDisableBreakpoint(location, isDisabled) { +function _removeOrDisableBreakpoint(location, isDisabled = false) { return ({ dispatch, getState, client }: ThunkArgs) => { let bp = getBreakpoint(getState(), location); if (!bp) { diff --git a/src/actions/sources.js b/src/actions/sources.js index c084404472..5e9d7ccca9 100644 --- a/src/actions/sources.js +++ b/src/actions/sources.js @@ -46,29 +46,38 @@ function checkSelectedSource(state, dispatch, source) { } } -function checkPendingBreakpoints(state, dispatch, source) { - const pendingBreakpoints = getPendingBreakpoints(state); - - if (pendingBreakpoints) { - pendingBreakpoints.forEach(pendingBreakpoint => { - const { - location: { line, sourceUrl, column }, - condition - } = pendingBreakpoint; - const sameSource = sourceUrl && sourceUrl == source.url; - - const location = { sourceId: source.id, sourceUrl, line, column }; +async function checkPendingBreakpoint( + state, + dispatch, + pendingBreakpoint, + source +) { + const { + location: { line, sourceUrl, column }, + condition + } = pendingBreakpoint; + const sameSource = sourceUrl && sourceUrl === source.url; + const location = { sourceId: source.id, sourceUrl, line, column }; + const bp = getBreakpoint(state, location); + + if (sameSource && !bp) { + if (location.column && isEnabled("columnBreakpoints")) { + await dispatch(addBreakpoint(location, { condition })); + } else { + await dispatch(addBreakpoint(location, { condition })); + } + } +} - const bp = getBreakpoint(state, location); +async function checkPendingBreakpoints(state, dispatch, source) { + const pendingBreakpoints = getPendingBreakpoints(state); + if (!pendingBreakpoints) { + return; + } - if (sameSource && !bp) { - if (location.column && isEnabled("columnBreakpoints")) { - dispatch(addBreakpoint(location, { condition })); - } else { - dispatch(addBreakpoint(location, { condition })); - } - } - }); + const pendingBreakpointsList = pendingBreakpoints.valueSeq().toJS(); + for (let pendingBreakpoint of pendingBreakpointsList) { + await checkPendingBreakpoint(state, dispatch, pendingBreakpoint, source); } } @@ -78,23 +87,27 @@ function checkPendingBreakpoints(state, dispatch, source) { * @static */ export function newSource(source: Source) { - return ({ dispatch, getState }: ThunkArgs) => { + return async ({ dispatch, getState }: ThunkArgs) => { if (prefs.clientSourceMapsEnabled) { - dispatch(loadSourceMap(source)); + await dispatch(loadSourceMap(source)); } dispatch({ type: constants.ADD_SOURCE, source }); checkSelectedSource(getState(), dispatch, source); - checkPendingBreakpoints(getState(), dispatch, source); + await checkPendingBreakpoints(getState(), dispatch, source); }; } export function newSources(sources: Source[]) { - return ({ dispatch, getState }: ThunkArgs) => { - sources - .filter(source => !getSource(getState(), source.id)) - .forEach(source => dispatch(newSource(source))); + return async ({ dispatch, getState }: ThunkArgs) => { + const filteredSources = sources.filter( + source => !getSource(getState(), source.id) + ); + + for (const source in filteredSources) { + await dispatch(newSource(source)); + } }; } diff --git a/src/actions/tests/breakpoints.js b/src/actions/tests/breakpoints.js index 5fa16bfc1d..0dc0d42d47 100644 --- a/src/actions/tests/breakpoints.js +++ b/src/actions/tests/breakpoints.js @@ -1,6 +1,33 @@ +// TODO: we would like to mock this in the local tests +const theMockedPendingBreakpoint = { + location: { + sourceId: "bar", + sourceUrl: "http://todomvc.com/bar.js", + line: 5, + column: undefined + }, + condition: "3", + disabled: false +}; + import { createStore, selectors, actions } from "../../utils/test-head"; +import { + makePendingLocationId, + makeLocationId +} from "../../reducers/breakpoints"; import expect from "expect.js"; +const slidingMockThreadClient = { + setBreakpoint: (location, condition) => { + return new Promise((resolve, reject) => { + const actualLocation = Object.assign({}, location, { + line: location.line + 2 + }); + resolve({ id: makeLocationId(location), actualLocation, condition }); + }); + } +}; + const simpleMockThreadClient = { setBreakpoint: (location, condition) => { return new Promise((resolve, reject) => { @@ -24,11 +51,40 @@ const simpleMockThreadClient = { describe("breakpoints", () => { it("should add a breakpoint", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); + const loc1 = { sourceId: "a", line: 5 }; + + await dispatch(actions.addBreakpoint(loc1)); + const bps = selectors.getBreakpoints(getState()); + const bp = selectors.getBreakpoint(getState(), loc1); + expect(bps.size).to.be(1); + expect(bp.location).to.eql(loc1); + }); + + it("should not re-add a breakpoint", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const loc1 = { sourceId: "a", line: 5 }; + + await dispatch(actions.addBreakpoint(loc1)); + let bps = selectors.getBreakpoints(getState()); + const bp = selectors.getBreakpoint(getState(), loc1); + expect(bps.size).to.be(1); + expect(bp.location).to.eql(loc1); + + await dispatch(actions.addBreakpoint(loc1)); + bps = selectors.getBreakpoints(getState()); + expect(bps.size).to.be(1); + }); - await dispatch(actions.addBreakpoint({ sourceId: "a", line: 5 })); - await dispatch(actions.addBreakpoint({ sourceId: "b", line: 6 })); + it("when the user adds a sliding breakpoint, a corresponding pending breakpoint should be added", async () => { + const { dispatch, getState } = createStore(slidingMockThreadClient); + const location = { sourceId: "a", line: 5 }; + const slidLocation = { sourceId: "a", line: 7 }; - expect(selectors.getBreakpoints(getState()).size).to.be(2); + await dispatch(actions.addBreakpoint(location)); + const bps = selectors.getBreakpoints(getState()); + const bp = selectors.getBreakpoint(getState(), slidLocation); + expect(bps.size).to.be(1); + expect(bp.location).to.eql(slidLocation); }); it("should remove a breakpoint", async () => { diff --git a/src/actions/tests/pending-breakpoints.js b/src/actions/tests/pending-breakpoints.js new file mode 100644 index 0000000000..03323e592a --- /dev/null +++ b/src/actions/tests/pending-breakpoints.js @@ -0,0 +1,248 @@ +// TODO: we would like to mock this in the local tests +const theMockedPendingBreakpoint = { + location: { + sourceUrl: "http://localhost:8000/examples/bar.js", + line: 5, + column: undefined + }, + condition: "3", + disabled: false +}; + +jest.mock("../../utils/prefs", () => ({ + prefs: { + expressions: [], + pendingBreakpoints: { + "http://localhost:8000/examples/bar.js:5:": { + location: { + sourceUrl: "http://localhost:8000/examples/bar.js", + line: 5, + column: undefined + }, + condition: "3", + disabled: false + } + } + } +})); + +import { + createStore, + selectors, + actions, + makeSource +} from "../../utils/test-head"; +import { + makeLocationId, + makePendingLocationId +} from "../../reducers/breakpoints"; +import expect from "expect.js"; + +function generateBreakpoint(filename) { + return { + location: { + sourceUrl: `http://localhost:8000/examples/${filename}`, + sourceId: filename, + line: 5 + }, + condition: null, + disabled: false + }; +} + +function generatePendingBreakpoint(breakpoint) { + const { + location: { sourceUrl, line, column }, + condition, + disabled + } = breakpoint; + + return { + location: { sourceUrl, line, column }, + condition, + disabled + }; +} + +function slideMockBp(bp) { + const slidBp = Object.assign({}, bp); + slidBp.location.line = bp.location.line + 2; + return slidBp; +} + +const slidingMockThreadClient = { + setBreakpoint: (location, condition) => { + return new Promise((resolve, reject) => { + const actualLocation = Object.assign({}, location, { + line: location.line + 2 + }); + resolve({ id: makeLocationId(location), actualLocation, condition }); + }); + } +}; + +const simpleMockThreadClient = { + setBreakpoint: (location, condition) => { + return new Promise((resolve, reject) => { + resolve({ id: "hi", actualLocation: location }); + }); + }, + + removeBreakpoint: id => { + return new Promise((resolve, reject) => { + resolve({ status: "done" }); + }); + }, + + setBreakpointCondition: (id, location, condition, noSliding) => { + return new Promise((resolve, reject) => { + resolve({ sourceId: "a", line: 5 }); + }); + } +}; + +describe("pending breakpoints", () => { + it("when the user adds a breakpoint, a corresponding pending breakpoint should be added", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const bp = generateBreakpoint("foo"); + const id = makePendingLocationId(bp.location); + + await dispatch(actions.addBreakpoint(bp.location)); + const pendingBps = selectors.getPendingBreakpoints(getState()); + expect(pendingBps.size).to.be(2); + expect(pendingBps.get(id)).to.eql(generatePendingBreakpoint(bp)); + }); + + it("when the user adds a sliding breakpoint, a corresponding pending breakpoint should be added", async () => { + const { dispatch, getState } = createStore(slidingMockThreadClient); + const bp = generateBreakpoint("foo"); + await dispatch(actions.addBreakpoint(bp.location)); + const pendingBps = selectors.getPendingBreakpoints(getState()); + + const slidBp = slideMockBp(bp); + const newId = makePendingLocationId(slidBp.location); + + const bps = selectors.getPendingBreakpoints(getState()); + const newBp = pendingBps.get(newId); + expect(newBp).to.eql(generatePendingBreakpoint(slidBp)); + }); + + describe("when two or more breakpoints are added", () => { + let breakpoint1; + let breakpoint2; + let breakpointId1; + let breakpointId2; + + beforeEach(() => { + breakpoint1 = generateBreakpoint("foo"); + breakpoint2 = generateBreakpoint("foo2"); + breakpointId1 = makePendingLocationId(breakpoint1.location); + breakpointId2 = makePendingLocationId(breakpoint2.location); + }); + + it("adds a corresponding pendingBreakpoint for each new addition", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + await dispatch(actions.addBreakpoint(breakpoint1.location)); + await dispatch(actions.addBreakpoint(breakpoint2.location)); + + const pendingBps = selectors.getPendingBreakpoints(getState()); + expect(pendingBps.get(breakpointId1)).to.eql( + generatePendingBreakpoint(breakpoint1) + ); + expect(pendingBps.get(breakpointId2)).to.eql( + generatePendingBreakpoint(breakpoint2) + ); + }); + + it("removes a corresponding pending breakpoint for each deletion", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + await dispatch(actions.addBreakpoint(breakpoint1.location)); + await dispatch(actions.addBreakpoint(breakpoint2.location)); + await dispatch(actions.removeBreakpoint(breakpoint1.location)); + + const pendingBps = selectors.getPendingBreakpoints(getState()); + expect(pendingBps.has(breakpointId1)).not.to.be(true); + expect(pendingBps.has(breakpointId2)).to.be(true); + }); + }); + + it("when the user disables a breakpoint, the corresponding pending breakpoint is also disabled", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const bp = generateBreakpoint("foo"); + const id = makePendingLocationId(bp.location); + + await dispatch(actions.addBreakpoint(bp.location)); + await dispatch(actions.disableBreakpoint(bp.location)); + const bps = selectors.getPendingBreakpoints(getState()); + const breakpoint = bps.get(id); + expect(breakpoint.disabled).to.be(true); + }); + + it("when the user updates a breakpoint, the corresponding pending breakpoints is updated", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const bp = generateBreakpoint("foo"); + const id = makePendingLocationId(bp.location); + + await dispatch(actions.addBreakpoint(bp.location)); + await dispatch( + actions.setBreakpointCondition(bp.location, { condition: "2" }) + ); + const bps = selectors.getPendingBreakpoints(getState()); + const breakpoint = bps.get(id); + expect(breakpoint.condition).to.be("2"); + }); + + it("when the user updates a breakpoint, the corresponding pending breakpoints are not removed", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const bp = generateBreakpoint("foo"); + const id = makePendingLocationId(bp.location); + + await dispatch(actions.addBreakpoint(bp.location)); + await dispatch( + actions.setBreakpointCondition(bp.location, { condition: "2" }) + ); + const bps = selectors.getPendingBreakpoints(getState()); + const breakpoint = bps.get(id); + expect(breakpoint.condition).to.be("2"); + }); + + it("when the debugger opens, it adds pending breakpoints", async () => { + const { getState } = createStore(simpleMockThreadClient); + const id = makePendingLocationId(theMockedPendingBreakpoint.location); + const bps = selectors.getPendingBreakpoints(getState()); + const bp = bps.get(id); + expect(bp).to.eql(generatePendingBreakpoint(theMockedPendingBreakpoint)); + }); + + it("when a bp is added, where there is a corresponding pending breakpoint we update it", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const bar = generateBreakpoint("bar.js"); + + await dispatch(actions.addBreakpoint(bar.location)); + + const bps = selectors.getPendingBreakpoints(getState()); + expect(bps.size).to.be(1); + }); + + it("when a bp is added, it does not remove the other pending breakpoints", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + const bp = generateBreakpoint("foo.js"); + + await dispatch(actions.addBreakpoint(bp.location)); + + const bps = selectors.getPendingBreakpoints(getState()); + expect(bps.size).to.be(2); + }); + + it("when a source is added, corresponding breakpoints are added as well", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + + let bps = selectors.getBreakpoints(getState()); + expect(bps.size).to.be(0); + + const source = makeSource("bar.js"); + await dispatch(actions.newSource(source)); + bps = selectors.getBreakpoints(getState()); + expect(bps.size).to.be(1); + }); +}); diff --git a/src/actions/tests/sources.js b/src/actions/tests/sources.js index 7faae59e86..6c61894287 100644 --- a/src/actions/tests/sources.js +++ b/src/actions/tests/sources.js @@ -12,6 +12,9 @@ const { getSourceText, getSourceTabs } = selectors; +import fromJS from "../../utils/fromJS"; +import I from "immutable"; +import { makePendingBreakpoint } from "../../reducers/breakpoints"; const threadClient = { sourceContents: function(sourceId) { diff --git a/src/client/firefox/commands.js b/src/client/firefox/commands.js index 7960da5cef..78218543bc 100644 --- a/src/client/firefox/commands.js +++ b/src/client/firefox/commands.js @@ -97,6 +97,7 @@ function onNewBreakpoint( ): BreakpointResult { const bpClient = res[1]; let actualLocation = res[0].actualLocation; + bpClients[bpClient.actor] = bpClient; // Firefox only returns `actualLocation` if it actually changed, @@ -105,6 +106,7 @@ function onNewBreakpoint( actualLocation = actualLocation ? { sourceId: actualLocation.source.actor, + sourceUrl: location.sourceUrl, line: actualLocation.line, column: actualLocation.column } diff --git a/src/reducers/breakpoints.js b/src/reducers/breakpoints.js index 4a191480c0..e685bca29a 100644 --- a/src/reducers/breakpoints.js +++ b/src/reducers/breakpoints.js @@ -20,7 +20,7 @@ import type { Record } from "../utils/makeRecord"; export type BreakpointsState = { breakpoints: I.Map, - pendingBreakpoints: ?I.Map, + pendingBreakpoints: any, breakpointsDisabled: false }; @@ -56,6 +56,12 @@ export function makeLocationId(location: Location) { return `${sourceId}:${line}:${column}`; } +export function makePendingLocationId(location: Location) { + let { sourceUrl, line, column } = location; + column = column || ""; + return `${sourceUrl}:${line}:${column}`; +} + function allBreakpointsDisabled(state) { return state.breakpoints.every(x => x.disabled); } @@ -64,9 +70,7 @@ function update(state: Record = State(), action: Action) { switch (action.type) { case "ADD_BREAKPOINT": { const newState = addBreakpoint(state, action); - if (newState) { - setPendingBreakpoints(newState); - } + setPendingBreakpoints(newState); return newState; } @@ -87,31 +91,9 @@ function update(state: Record = State(), action: Action) { } case "SET_BREAKPOINT_CONDITION": { - const id = makeLocationId(action.breakpoint.location); - - if (action.status === "start") { - const bp = state.breakpoints.get(id); - return state.setIn( - ["breakpoints", id], - updateObj(bp, { - loading: true, - condition: action.condition - }) - ); - } else if (action.status === "done") { - const bp = state.breakpoints.get(id); - return state.setIn( - ["breakpoints", id], - updateObj(bp, { - id: action.value.id, - loading: false - }) - ); - } else if (action.status === "error") { - return state.deleteIn(["breakpoints", id]); - } - - break; + const newState = setCondition(state, action); + setPendingBreakpoints(newState); + return newState; } } @@ -120,11 +102,10 @@ function update(state: Record = State(), action: Action) { function addBreakpoint(state, action) { const id = makeLocationId(action.breakpoint.location); - if (action.status === "start") { let bp = state.breakpoints.get(id) || action.breakpoint; - return state + const updatedState = state .setIn( ["breakpoints", id], updateObj(bp, { @@ -137,6 +118,8 @@ function addBreakpoint(state, action) { }) ) .set("breakpointsDisabled", false); + + return updatedState; } if (action.status === "done") { @@ -146,19 +129,13 @@ function addBreakpoint(state, action) { // If the breakpoint moved, update the map if (locationMoved(location, actualLocation)) { - state = state.deleteIn(["breakpoints", id]); - - const movedId = makeLocationId(actualLocation); - const currentBp = - state.breakpoints.get(movedId) || fromJS(action.breakpoint); - const newBp = updateObj(currentBp, { location: actualLocation }); - state = state.setIn(["breakpoints", movedId], newBp); + state = slideBreakpoint(state, action); location = actualLocation; } const locationId = makeLocationId(location); const bp = state.breakpoints.get(locationId); - return state.setIn( + const updatedState = state.setIn( ["breakpoints", locationId], updateObj(bp, { id: breakpointId, @@ -167,6 +144,8 @@ function addBreakpoint(state, action) { text: text }) ); + + return updatePendingBreakpoint(updatedState, bp); } if (action.status === "error") { @@ -176,55 +155,101 @@ function addBreakpoint(state, action) { } function removeBreakpoint(state, action) { - if (action.status === "done") { - const id = makeLocationId(action.breakpoint.location); + if (action.status != "done") { + return state; + } - if (action.disabled) { - const bp = state.breakpoints.get(id); - const updatedState = state.setIn( - ["breakpoints", id], - updateObj(bp, { - loading: false, - disabled: true - }) - ); + const id = makeLocationId(action.breakpoint.location); + const pendingId = makePendingLocationId(action.breakpoint.location); + let updatedState = undefined; + + if (action.disabled) { + const bp = state.breakpoints.get(id); + const breakpoint = updateObj(bp, { + loading: false, + disabled: true + }); + updatedState = state.setIn(["breakpoints", id], breakpoint); + updatedState = updatePendingBreakpoint(updatedState, breakpoint); + } else { + updatedState = state + .deleteIn(["breakpoints", id]) + .deleteIn(["pendingBreakpoints", pendingId]); + } - return updatedState.set( - "breakpointsDisabled", - allBreakpointsDisabled(updatedState) - ); - } + return updatedState.set( + "breakpointsDisabled", + allBreakpointsDisabled(updatedState) + ); +} - const updatedState = state.deleteIn(["breakpoints", id]); +function setCondition(state, action) { + const id = makeLocationId(action.breakpoint.location); - return updatedState.set( - "breakpointsDisabled", - allBreakpointsDisabled(updatedState) + if (action.status === "start") { + const bp = state.breakpoints.get(id); + return state.setIn( + ["breakpoints", id], + updateObj(bp, { + loading: true, + condition: action.condition + }) ); } - return state; + if (action.status === "done") { + const bp = state.breakpoints.get(id); + const updatedBreakpoint = updateObj(bp, { + id: action.value.id, + loading: false + }); + + let updatedState = state.setIn(["breakpoints", id], updatedBreakpoint); + + return updatePendingBreakpoint(updatedState, updatedBreakpoint); + } + + if (action.status === "error") { + return state.deleteIn(["breakpoints", id]); + } +} + +function slideBreakpoint(state, action) { + const { actualLocation, id } = action.value; + const { breakpoint } = action; + const currentBp = state.breakpoints.get(id) || fromJS(breakpoint); + + const locationId = makeLocationId(breakpoint.location); + const movedLocationId = makeLocationId(actualLocation); + state = state.deleteIn(["breakpoints", locationId]); + + return state.setIn( + ["breakpoints", movedLocationId], + updateObj(currentBp, { location: actualLocation }) + ); } -function makePendingBreakpoint(bp: any) { +export function makePendingBreakpoint(bp: any) { const { location: { sourceUrl, line, column }, condition, disabled } = bp; const location = { sourceUrl, line, column }; return { condition, disabled, location }; } -function filterByNotLoading(bp: any): boolean { - return !bp.loading; +function setPendingBreakpoints(state) { + prefs.pendingBreakpoints = state.pendingBreakpoints; } -function setPendingBreakpoints(state) { - prefs.pendingBreakpoints = Object.values(state.get("breakpoints").toJS()) - .filter(filterByNotLoading) - .map(makePendingBreakpoint); +function updatePendingBreakpoint(state, breakpoint) { + const id = makePendingLocationId(breakpoint.location); + return state.setIn( + ["pendingBreakpoints", id], + makePendingBreakpoint(breakpoint) + ); } function restorePendingBreakpoints() { - return prefs.pendingBreakpoints; + return I.Map(prefs.pendingBreakpoints); } // Selectors diff --git a/src/test/tests-setup.js b/src/test/tests-setup.js index 2803211773..0704341bfa 100644 --- a/src/test/tests-setup.js +++ b/src/test/tests-setup.js @@ -1,5 +1,4 @@ global.Worker = require("workerjs"); - global.L10N = { getStr: () => {} }; const path = require("path"); From 5cf7147e13cdb05209493b18f53f50dd8ad01058 Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Fri, 12 May 2017 13:44:02 +0100 Subject: [PATCH 02/11] update newSources --- src/actions/sources.js | 2 +- src/actions/tests/pending-breakpoints.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/actions/sources.js b/src/actions/sources.js index 5e9d7ccca9..db11dd4a08 100644 --- a/src/actions/sources.js +++ b/src/actions/sources.js @@ -105,7 +105,7 @@ export function newSources(sources: Source[]) { source => !getSource(getState(), source.id) ); - for (const source in filteredSources) { + for (let source of filteredSources) { await dispatch(newSource(source)); } }; diff --git a/src/actions/tests/pending-breakpoints.js b/src/actions/tests/pending-breakpoints.js index 03323e592a..e434af8f48 100644 --- a/src/actions/tests/pending-breakpoints.js +++ b/src/actions/tests/pending-breakpoints.js @@ -245,4 +245,17 @@ describe("pending breakpoints", () => { bps = selectors.getBreakpoints(getState()); expect(bps.size).to.be(1); }); + + it("when a sources are added, corresponding breakpoints are added as well", async () => { + const { dispatch, getState } = createStore(simpleMockThreadClient); + + let bps = selectors.getBreakpoints(getState()); + expect(bps.size).to.be(0); + + const source1 = makeSource("bar.js"); + const source2 = makeSource("foo.js"); + await dispatch(actions.newSources([source1, source2])); + bps = selectors.getBreakpoints(getState()); + expect(bps.size).to.be(1); + }); }); From 2728a928c8f2050894fd91eedd757b07722c2a7c Mon Sep 17 00:00:00 2001 From: Yulia Startsev Date: Mon, 15 May 2017 10:47:11 +0200 Subject: [PATCH 03/11] address comments in reducers/breakpoints.js --- flow-typed/debugger-html.js | 14 ++++++++++ src/actions/sources.js | 4 +-- src/reducers/breakpoints.js | 51 ++++++++++++++++++++----------------- 3 files changed, 44 insertions(+), 25 deletions(-) diff --git a/flow-typed/debugger-html.js b/flow-typed/debugger-html.js index f9acdc3391..2e1beb4473 100644 --- a/flow-typed/debugger-html.js +++ b/flow-typed/debugger-html.js @@ -64,6 +64,20 @@ declare module "debugger-html" { actualLocation: Location }; + /** + * PendingBreakpoint + * + * @memberof types + * @static + */ + declare type PendingBreakpoint = { + location: Location, + loading: boolean, + disabled: boolean, + text: string, + condition: ?string + }; + /** * Frame ID * diff --git a/src/actions/sources.js b/src/actions/sources.js index db11dd4a08..5f91f72442 100644 --- a/src/actions/sources.js +++ b/src/actions/sources.js @@ -75,8 +75,8 @@ async function checkPendingBreakpoints(state, dispatch, source) { return; } - const pendingBreakpointsList = pendingBreakpoints.valueSeq().toJS(); - for (let pendingBreakpoint of pendingBreakpointsList) { + const pendingBreakpointsArray = pendingBreakpoints.valueSeq().toJS(); + for (let pendingBreakpoint of pendingBreakpointsArray) { await checkPendingBreakpoint(state, dispatch, pendingBreakpoint, source); } } diff --git a/src/reducers/breakpoints.js b/src/reducers/breakpoints.js index e685bca29a..b87ae3efbc 100644 --- a/src/reducers/breakpoints.js +++ b/src/reducers/breakpoints.js @@ -14,13 +14,13 @@ import * as I from "immutable"; import makeRecord from "../utils/makeRecord"; import { prefs } from "../utils/prefs"; -import type { Breakpoint, Location } from "../types"; +import type { Breakpoint, PendingBreakpoint, Location } from "../types"; import type { Action } from "../actions/types"; import type { Record } from "../utils/makeRecord"; export type BreakpointsState = { breakpoints: I.Map, - pendingBreakpoints: any, + pendingBreakpoints: I.Map, breakpointsDisabled: false }; @@ -51,9 +51,9 @@ function locationMoved(location, newLocation) { } export function makeLocationId(location: Location) { - let { sourceId, line, column } = location; - column = column || ""; - return `${sourceId}:${line}:${column}`; + const { sourceId, line, column } = location; + const columnString = column || ""; + return `${sourceId}:${line}:${columnString}`; } export function makePendingLocationId(location: Location) { @@ -75,7 +75,7 @@ function update(state: Record = State(), action: Action) { } case "REMOVE_BREAKPOINT": { - const newState = removeBreakpoint(state, action); + const newState = removeOrDisableBreakpoint(state, action); setPendingBreakpoints(newState); return newState; } @@ -154,28 +154,33 @@ function addBreakpoint(state, action) { } } -function removeBreakpoint(state, action) { +function disableBreakpoint(state, id) { + const bp = state.breakpoints.get(id); + const breakpoint = updateObj(bp, { + loading: false, + disabled: true + }); + const updatedState = state.setIn(["breakpoints", id], breakpoint); + return updatePendingBreakpoint(updatedState, breakpoint); +} + +function deleteBreakpoint(state, id, pendingId) { + return state + .deleteIn(["breakpoints", id]) + .deleteIn(["pendingBreakpoints", pendingId]); +} + +function removeOrDisableBreakpoint(state, action) { if (action.status != "done") { return state; } const id = makeLocationId(action.breakpoint.location); const pendingId = makePendingLocationId(action.breakpoint.location); - let updatedState = undefined; - if (action.disabled) { - const bp = state.breakpoints.get(id); - const breakpoint = updateObj(bp, { - loading: false, - disabled: true - }); - updatedState = state.setIn(["breakpoints", id], breakpoint); - updatedState = updatePendingBreakpoint(updatedState, breakpoint); - } else { - updatedState = state - .deleteIn(["breakpoints", id]) - .deleteIn(["pendingBreakpoints", pendingId]); - } + const updatedState = action.disabled + ? disableBreakpoint(state, id) + : deleteBreakpoint(state, id, pendingId); return updatedState.set( "breakpointsDisabled", @@ -221,9 +226,9 @@ function slideBreakpoint(state, action) { const locationId = makeLocationId(breakpoint.location); const movedLocationId = makeLocationId(actualLocation); - state = state.deleteIn(["breakpoints", locationId]); + const updatedState = state.deleteIn(["breakpoints", locationId]); - return state.setIn( + return updatedState.setIn( ["breakpoints", movedLocationId], updateObj(currentBp, { location: actualLocation }) ); From b0f11bf7173bf463232fb1eebe33a219857ee0fa Mon Sep 17 00:00:00 2001 From: Yulia Startsev Date: Mon, 15 May 2017 11:37:05 +0200 Subject: [PATCH 04/11] add helpers to breakpoints --- package.json | 1 + src/actions/tests/breakpoints.js | 76 +++++++----------------- src/actions/tests/helpers/breakpoints.js | 57 ++++++++++++++++++ 3 files changed, 78 insertions(+), 56 deletions(-) create mode 100644 src/actions/tests/helpers/breakpoints.js diff --git a/package.json b/package.json index f7d59c1be5..fd9e3c6f74 100644 --- a/package.json +++ b/package.json @@ -155,6 +155,7 @@ ], "testPathIgnorePatterns": [ "/node_modules/", + "/helpers/", "/test/", "/utils/tests/fixtures/" ], diff --git a/src/actions/tests/breakpoints.js b/src/actions/tests/breakpoints.js index 0dc0d42d47..532a789294 100644 --- a/src/actions/tests/breakpoints.js +++ b/src/actions/tests/breakpoints.js @@ -1,52 +1,10 @@ -// TODO: we would like to mock this in the local tests -const theMockedPendingBreakpoint = { - location: { - sourceId: "bar", - sourceUrl: "http://todomvc.com/bar.js", - line: 5, - column: undefined - }, - condition: "3", - disabled: false -}; +import expect from "expect.js"; import { createStore, selectors, actions } from "../../utils/test-head"; import { - makePendingLocationId, - makeLocationId -} from "../../reducers/breakpoints"; -import expect from "expect.js"; - -const slidingMockThreadClient = { - setBreakpoint: (location, condition) => { - return new Promise((resolve, reject) => { - const actualLocation = Object.assign({}, location, { - line: location.line + 2 - }); - resolve({ id: makeLocationId(location), actualLocation, condition }); - }); - } -}; - -const simpleMockThreadClient = { - setBreakpoint: (location, condition) => { - return new Promise((resolve, reject) => { - resolve({ id: "hi", actualLocation: location }); - }); - }, - - removeBreakpoint: id => { - return new Promise((resolve, reject) => { - resolve({ status: "done" }); - }); - }, - - setBreakpointCondition: (id, location, condition, noSliding) => { - return new Promise((resolve, reject) => { - resolve({ sourceId: "a", line: 5 }); - }); - } -}; + simulateCorrectThreadClient, + simpleMockThreadClient +} from "./helpers/breakpoints.js"; describe("breakpoints", () => { it("should add a breakpoint", async () => { @@ -75,16 +33,22 @@ describe("breakpoints", () => { expect(bps.size).to.be(1); }); - it("when the user adds a sliding breakpoint, a corresponding pending breakpoint should be added", async () => { - const { dispatch, getState } = createStore(slidingMockThreadClient); - const location = { sourceId: "a", line: 5 }; - const slidLocation = { sourceId: "a", line: 7 }; - - await dispatch(actions.addBreakpoint(location)); - const bps = selectors.getBreakpoints(getState()); - const bp = selectors.getBreakpoint(getState(), slidLocation); - expect(bps.size).to.be(1); - expect(bp.location).to.eql(slidLocation); + describe("adding a breakpoint to an invalid location", async () => { + it("adds only one breakpoint with a corrected location", async () => { + const invalidLocation = { sourceId: "a", line: 5 }; + const { + correctedThreadClient, + correctedLocation + } = simulateCorrectThreadClient(2, invalidLocation); + const { dispatch, getState } = createStore(correctedThreadClient); + + await dispatch(actions.addBreakpoint(invalidLocation)); + const state = getState(); + const bps = selectors.getBreakpoints(state); + const bp = selectors.getBreakpoint(state, correctedLocation); + expect(bps.size).to.be(1); + expect(bp.location).to.eql(correctedLocation); + }); }); it("should remove a breakpoint", async () => { diff --git a/src/actions/tests/helpers/breakpoints.js b/src/actions/tests/helpers/breakpoints.js new file mode 100644 index 0000000000..0665c19b6a --- /dev/null +++ b/src/actions/tests/helpers/breakpoints.js @@ -0,0 +1,57 @@ +import { makeLocationId } from "../../../reducers/breakpoints"; + +export const theMockedPendingBreakpoint = { + location: { + sourceId: "bar", + sourceUrl: "http://todomvc.com/bar.js", + line: 5, + column: undefined + }, + condition: "3", + disabled: false +}; + +function generateCorrectingThreadClient(offset = 0) { + return { + setBreakpoint: (location, condition) => { + return new Promise((resolve, reject) => { + const actualLocation = Object.assign({}, location, { + line: location.line + offset + }); + resolve({ id: makeLocationId(location), actualLocation, condition }); + }); + } + }; +} + +/* in some cases, a breakpoint may be added, but the source will respond + * with a different breakpoint location. This is due to the breakpoint being + * added between functions, or somewhere that doesnt make sense. This function + * simulates that behavior. + * */ +export function simulateCorrectThreadClient(offset, location) { + const correctedThreadClient = generateCorrectingThreadClient(offset); + const offsetLine = { line: location.line + offset }; + const correctedLocation = Object.assign({}, location, offsetLine); + return { correctedThreadClient, correctedLocation }; +} + +export const simpleMockThreadClient = { + setBreakpoint: (location, condition) => { + return new Promise((resolve, reject) => { + resolve({ id: "hi", actualLocation: location }); + }); + }, + + removeBreakpoint: id => { + return new Promise((resolve, reject) => { + resolve({ status: "done" }); + }); + }, + + setBreakpointCondition: (id, location, condition, noSliding) => { + return new Promise((resolve, reject) => { + resolve({ sourceId: "a", line: 5 }); + }); + } +}; From 1fb5dd6100b31810b80b05f3a2a72a99b1349183 Mon Sep 17 00:00:00 2001 From: Yulia Startsev Date: Mon, 15 May 2017 12:27:25 +0200 Subject: [PATCH 05/11] add async to newsource actions in actions/sources tests --- src/actions/tests/sources.js | 79 +++++++++++++++++------------------- 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/src/actions/tests/sources.js b/src/actions/tests/sources.js index 6c61894287..132ae83eec 100644 --- a/src/actions/tests/sources.js +++ b/src/actions/tests/sources.js @@ -12,9 +12,6 @@ const { getSourceText, getSourceTabs } = selectors; -import fromJS from "../../utils/fromJS"; -import I from "immutable"; -import { makePendingBreakpoint } from "../../reducers/breakpoints"; const threadClient = { sourceContents: function(sourceId) { @@ -42,10 +39,10 @@ const threadClient = { process.on("unhandledRejection", (reason, p) => {}); describe("sources", () => { - it("should add sources to state", () => { + it("should add sources to state", async () => { const { dispatch, getState } = createStore(); - dispatch(actions.newSource(makeSource("base.js"))); - dispatch(actions.newSource(makeSource("jquery.js"))); + await dispatch(actions.newSource(makeSource("base.js"))); + await dispatch(actions.newSource(makeSource("jquery.js"))); expect(getSources(getState()).size).to.equal(2); const base = getSource(getState(), "base.js"); @@ -54,29 +51,29 @@ describe("sources", () => { expect(jquery.get("id")).to.equal("jquery.js"); }); - it("should select a source", () => { + it("should select a source", async () => { // Note that we pass an empty client in because the action checks // if it exists. const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); dispatch(actions.selectSource("foo.js")); expect(getSelectedSource(getState()).get("id")).to.equal("foo.js"); }); - it("should automatically select a pending source", () => { + it("should automatically select a pending source", async () => { const { dispatch, getState } = createStore(threadClient); const baseSource = makeSource("base.js"); dispatch(actions.selectSourceURL(baseSource.url)); expect(getSelectedSource(getState())).to.be(undefined); - dispatch(actions.newSource(baseSource)); + await dispatch(actions.newSource(baseSource)); expect(getSelectedSource(getState()).get("url")).to.be(baseSource.url); }); - it("should open a tab for the source", () => { + it("should open a tab for the source", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); dispatch(actions.selectSource("foo.js")); const tabs = getSourceTabs(getState()); @@ -84,11 +81,11 @@ describe("sources", () => { expect(tabs.get(0)).to.equal("http://localhost:8000/examples/foo.js"); }); - it("should select previous tab on tab closed", () => { + it("should select previous tab on tab closed", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); - dispatch(actions.newSource(makeSource("baz.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("baz.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch(actions.selectSource("baz.js")); @@ -97,11 +94,11 @@ describe("sources", () => { expect(getSourceTabs(getState()).size).to.be(2); }); - it("should select next tab on tab closed if no previous tab", () => { + it("should select next tab on tab closed if no previous tab", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); - dispatch(actions.newSource(makeSource("baz.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("baz.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch(actions.selectSource("baz.js")); @@ -154,9 +151,9 @@ describe("sources", () => { }); describe("closing tabs", () => { - it("closing a tab", () => { + it("closing a tab", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.closeTab("http://localhost:8000/examples/foo.js")); @@ -164,10 +161,10 @@ describe("closing tabs", () => { expect(getSourceTabs(getState()).size).to.be(0); }); - it("closing the inactive tab", () => { + it("closing the inactive tab", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch(actions.closeTab("http://localhost:8000/examples/foo.js")); @@ -176,9 +173,9 @@ describe("closing tabs", () => { expect(getSourceTabs(getState()).size).to.be(1); }); - it("closing the only tab", () => { + it("closing the only tab", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.closeTab("http://localhost:8000/examples/foo.js")); @@ -186,10 +183,10 @@ describe("closing tabs", () => { expect(getSourceTabs(getState()).size).to.be(0); }); - it("closing the active tab", () => { + it("closing the active tab", async () => { const { dispatch, getState } = createStore(threadClient); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch(actions.closeTab("http://localhost:8000/examples/bar.js")); @@ -198,11 +195,11 @@ describe("closing tabs", () => { expect(getSourceTabs(getState()).size).to.be(1); }); - it("closing many inactive tabs", () => { + it("closing many inactive tabs", async () => { const { dispatch, getState } = createStore({}); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); - dispatch(actions.newSource(makeSource("bazz.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("bazz.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch(actions.selectSource("bazz.js")); @@ -217,11 +214,11 @@ describe("closing tabs", () => { expect(getSourceTabs(getState()).size).to.be(1); }); - it("closing many tabs including the active tab", () => { + it("closing many tabs including the active tab", async () => { const { dispatch, getState } = createStore({}); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); - dispatch(actions.newSource(makeSource("bazz.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("bazz.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch(actions.selectSource("bazz.js")); @@ -236,10 +233,10 @@ describe("closing tabs", () => { expect(getSourceTabs(getState()).size).to.be(1); }); - it("closing all the tabs", () => { + it("closing all the tabs", async () => { const { dispatch, getState } = createStore({}); - dispatch(actions.newSource(makeSource("foo.js"))); - dispatch(actions.newSource(makeSource("bar.js"))); + await dispatch(actions.newSource(makeSource("foo.js"))); + await dispatch(actions.newSource(makeSource("bar.js"))); dispatch(actions.selectSource("foo.js")); dispatch(actions.selectSource("bar.js")); dispatch( From f007e955bf40a1106dfd550b08491d9f6ab81c3d Mon Sep 17 00:00:00 2001 From: Yulia Startsev Date: Mon, 15 May 2017 13:08:16 +0200 Subject: [PATCH 06/11] refactor actions/tests/pending-breakpoints for readability --- src/actions/tests/helpers/breakpoints.js | 39 +++++- src/actions/tests/pending-breakpoints.js | 163 ++++++++--------------- 2 files changed, 88 insertions(+), 114 deletions(-) diff --git a/src/actions/tests/helpers/breakpoints.js b/src/actions/tests/helpers/breakpoints.js index 0665c19b6a..cdc3f6fd5b 100644 --- a/src/actions/tests/helpers/breakpoints.js +++ b/src/actions/tests/helpers/breakpoints.js @@ -2,8 +2,7 @@ import { makeLocationId } from "../../../reducers/breakpoints"; export const theMockedPendingBreakpoint = { location: { - sourceId: "bar", - sourceUrl: "http://todomvc.com/bar.js", + sourceUrl: "http://localhost:8000/examples/bar.js", line: 5, column: undefined }, @@ -11,6 +10,10 @@ export const theMockedPendingBreakpoint = { disabled: false }; +export function generateCorrectedBreakpoint(breakpoint, correctedLocation) { + return Object.assign({}, breakpoint, { location: correctedLocation }); +} + function generateCorrectingThreadClient(offset = 0) { return { setBreakpoint: (location, condition) => { @@ -36,20 +39,46 @@ export function simulateCorrectThreadClient(offset, location) { return { correctedThreadClient, correctedLocation }; } +export function generateBreakpoint(filename) { + return { + location: { + sourceUrl: `http://localhost:8000/examples/${filename}`, + sourceId: filename, + line: 5 + }, + condition: null, + disabled: false + }; +} + +export function generatePendingBreakpoint(breakpoint) { + const { + location: { sourceUrl, line, column }, + condition, + disabled + } = breakpoint; + + return { + location: { sourceUrl, line, column }, + condition, + disabled + }; +} + export const simpleMockThreadClient = { - setBreakpoint: (location, condition) => { + setBreakpoint: (location, _condition) => { return new Promise((resolve, reject) => { resolve({ id: "hi", actualLocation: location }); }); }, - removeBreakpoint: id => { + removeBreakpoint: _id => { return new Promise((resolve, reject) => { resolve({ status: "done" }); }); }, - setBreakpointCondition: (id, location, condition, noSliding) => { + setBreakpointCondition: (_id, _location, _condition, _noSliding) => { return new Promise((resolve, reject) => { resolve({ sourceId: "a", line: 5 }); }); diff --git a/src/actions/tests/pending-breakpoints.js b/src/actions/tests/pending-breakpoints.js index e434af8f48..3feee4fc7e 100644 --- a/src/actions/tests/pending-breakpoints.js +++ b/src/actions/tests/pending-breakpoints.js @@ -1,13 +1,12 @@ // TODO: we would like to mock this in the local tests -const theMockedPendingBreakpoint = { - location: { - sourceUrl: "http://localhost:8000/examples/bar.js", - line: 5, - column: undefined - }, - condition: "3", - disabled: false -}; +import { + generatePendingBreakpoint, + generateBreakpoint, + theMockedPendingBreakpoint, + simulateCorrectThreadClient, + generateCorrectedBreakpoint, + simpleMockThreadClient +} from "./helpers/breakpoints.js"; jest.mock("../../utils/prefs", () => ({ prefs: { @@ -32,77 +31,11 @@ import { actions, makeSource } from "../../utils/test-head"; -import { - makeLocationId, - makePendingLocationId -} from "../../reducers/breakpoints"; +import { makePendingLocationId } from "../../reducers/breakpoints"; import expect from "expect.js"; -function generateBreakpoint(filename) { - return { - location: { - sourceUrl: `http://localhost:8000/examples/${filename}`, - sourceId: filename, - line: 5 - }, - condition: null, - disabled: false - }; -} - -function generatePendingBreakpoint(breakpoint) { - const { - location: { sourceUrl, line, column }, - condition, - disabled - } = breakpoint; - - return { - location: { sourceUrl, line, column }, - condition, - disabled - }; -} - -function slideMockBp(bp) { - const slidBp = Object.assign({}, bp); - slidBp.location.line = bp.location.line + 2; - return slidBp; -} - -const slidingMockThreadClient = { - setBreakpoint: (location, condition) => { - return new Promise((resolve, reject) => { - const actualLocation = Object.assign({}, location, { - line: location.line + 2 - }); - resolve({ id: makeLocationId(location), actualLocation, condition }); - }); - } -}; - -const simpleMockThreadClient = { - setBreakpoint: (location, condition) => { - return new Promise((resolve, reject) => { - resolve({ id: "hi", actualLocation: location }); - }); - }, - - removeBreakpoint: id => { - return new Promise((resolve, reject) => { - resolve({ status: "done" }); - }); - }, - - setBreakpointCondition: (id, location, condition, noSliding) => { - return new Promise((resolve, reject) => { - resolve({ sourceId: "a", line: 5 }); - }); - } -}; - -describe("pending breakpoints", () => { - it("when the user adds a breakpoint, a corresponding pending breakpoint should be added", async () => { +describe("when adding breakpoints", () => { + it("a corresponding pending breakpoint should be added", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); const bp = generateBreakpoint("foo"); const id = makePendingLocationId(bp.location); @@ -113,21 +46,7 @@ describe("pending breakpoints", () => { expect(pendingBps.get(id)).to.eql(generatePendingBreakpoint(bp)); }); - it("when the user adds a sliding breakpoint, a corresponding pending breakpoint should be added", async () => { - const { dispatch, getState } = createStore(slidingMockThreadClient); - const bp = generateBreakpoint("foo"); - await dispatch(actions.addBreakpoint(bp.location)); - const pendingBps = selectors.getPendingBreakpoints(getState()); - - const slidBp = slideMockBp(bp); - const newId = makePendingLocationId(slidBp.location); - - const bps = selectors.getPendingBreakpoints(getState()); - const newBp = pendingBps.get(newId); - expect(newBp).to.eql(generatePendingBreakpoint(slidBp)); - }); - - describe("when two or more breakpoints are added", () => { + describe("adding and deleting breakpoints", () => { let breakpoint1; let breakpoint2; let breakpointId1; @@ -140,7 +59,7 @@ describe("pending breakpoints", () => { breakpointId2 = makePendingLocationId(breakpoint2.location); }); - it("adds a corresponding pendingBreakpoint for each new addition", async () => { + it("add a corresponding pendingBreakpoint for each addition", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); await dispatch(actions.addBreakpoint(breakpoint1.location)); await dispatch(actions.addBreakpoint(breakpoint2.location)); @@ -154,7 +73,7 @@ describe("pending breakpoints", () => { ); }); - it("removes a corresponding pending breakpoint for each deletion", async () => { + it("remove a corresponding pending breakpoint when deleting", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); await dispatch(actions.addBreakpoint(breakpoint1.location)); await dispatch(actions.addBreakpoint(breakpoint2.location)); @@ -165,34 +84,36 @@ describe("pending breakpoints", () => { expect(pendingBps.has(breakpointId2)).to.be(true); }); }); +}); - it("when the user disables a breakpoint, the corresponding pending breakpoint is also disabled", async () => { +describe("when changing an existing breakpoint", () => { + it("updates corresponding pendingBreakpoint", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); const bp = generateBreakpoint("foo"); const id = makePendingLocationId(bp.location); await dispatch(actions.addBreakpoint(bp.location)); - await dispatch(actions.disableBreakpoint(bp.location)); + await dispatch( + actions.setBreakpointCondition(bp.location, { condition: "2" }) + ); const bps = selectors.getPendingBreakpoints(getState()); const breakpoint = bps.get(id); - expect(breakpoint.disabled).to.be(true); + expect(breakpoint.condition).to.be("2"); }); - it("when the user updates a breakpoint, the corresponding pending breakpoints is updated", async () => { + it("if disabled, updates corresponding pendingBreakpoint", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); const bp = generateBreakpoint("foo"); const id = makePendingLocationId(bp.location); await dispatch(actions.addBreakpoint(bp.location)); - await dispatch( - actions.setBreakpointCondition(bp.location, { condition: "2" }) - ); + await dispatch(actions.disableBreakpoint(bp.location)); const bps = selectors.getPendingBreakpoints(getState()); const breakpoint = bps.get(id); - expect(breakpoint.condition).to.be("2"); + expect(breakpoint.disabled).to.be(true); }); - it("when the user updates a breakpoint, the corresponding pending breakpoints are not removed", async () => { + it("does not delete the pre-existing pendingBreakpoint", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); const bp = generateBreakpoint("foo"); const id = makePendingLocationId(bp.location); @@ -205,8 +126,10 @@ describe("pending breakpoints", () => { const breakpoint = bps.get(id); expect(breakpoint.condition).to.be("2"); }); +}); - it("when the debugger opens, it adds pending breakpoints", async () => { +describe("initializing when pending breakpoints exist in perfs", () => { + it("syncs pending breakpoints", async () => { const { getState } = createStore(simpleMockThreadClient); const id = makePendingLocationId(theMockedPendingBreakpoint.location); const bps = selectors.getPendingBreakpoints(getState()); @@ -214,7 +137,7 @@ describe("pending breakpoints", () => { expect(bp).to.eql(generatePendingBreakpoint(theMockedPendingBreakpoint)); }); - it("when a bp is added, where there is a corresponding pending breakpoint we update it", async () => { + it("readding breakpoints update existing pending breakpoints", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); const bar = generateBreakpoint("bar.js"); @@ -224,7 +147,7 @@ describe("pending breakpoints", () => { expect(bps.size).to.be(1); }); - it("when a bp is added, it does not remove the other pending breakpoints", async () => { + it("adding bps doesn't remove existing pending breakpoints", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); const bp = generateBreakpoint("foo.js"); @@ -233,8 +156,10 @@ describe("pending breakpoints", () => { const bps = selectors.getPendingBreakpoints(getState()); expect(bps.size).to.be(2); }); +}); - it("when a source is added, corresponding breakpoints are added as well", async () => { +describe("adding sources", () => { + it("corresponding breakpoints are added for a single source", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); let bps = selectors.getBreakpoints(getState()); @@ -246,7 +171,7 @@ describe("pending breakpoints", () => { expect(bps.size).to.be(1); }); - it("when a sources are added, corresponding breakpoints are added as well", async () => { + it("add corresponding breakpoints for multiple sources", async () => { const { dispatch, getState } = createStore(simpleMockThreadClient); let bps = selectors.getBreakpoints(getState()); @@ -259,3 +184,23 @@ describe("pending breakpoints", () => { expect(bps.size).to.be(1); }); }); + +describe("invalid breakpoint location", () => { + it("a corrected corresponding pending breakpoint is added", async () => { + // setup + const bp = generateBreakpoint("foo"); + const { + correctedThreadClient, + correctedLocation + } = simulateCorrectThreadClient(2, bp.location); + const { dispatch, getState } = createStore(correctedThreadClient); + const slidBp = generateCorrectedBreakpoint(bp, correctedLocation); + const correctedPendingId = makePendingLocationId(correctedLocation); + + // test + await dispatch(actions.addBreakpoint(bp.location)); + const pendingBps = selectors.getPendingBreakpoints(getState()); + const pendingBp = pendingBps.get(correctedPendingId); + expect(pendingBp).to.eql(generatePendingBreakpoint(slidBp)); + }); +}); From 1f86764432d113faf140d198460381c0ead6a59f Mon Sep 17 00:00:00 2001 From: Yulia Startsev Date: Mon, 15 May 2017 13:11:29 +0200 Subject: [PATCH 07/11] prefer Promise.resolve to new Promise --- src/actions/tests/helpers/breakpoints.js | 33 ++++++++++-------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/actions/tests/helpers/breakpoints.js b/src/actions/tests/helpers/breakpoints.js index cdc3f6fd5b..f524f2905e 100644 --- a/src/actions/tests/helpers/breakpoints.js +++ b/src/actions/tests/helpers/breakpoints.js @@ -17,11 +17,14 @@ export function generateCorrectedBreakpoint(breakpoint, correctedLocation) { function generateCorrectingThreadClient(offset = 0) { return { setBreakpoint: (location, condition) => { - return new Promise((resolve, reject) => { - const actualLocation = Object.assign({}, location, { - line: location.line + offset - }); - resolve({ id: makeLocationId(location), actualLocation, condition }); + const actualLocation = Object.assign({}, location, { + line: location.line + offset + }); + + return Promise.resolve({ + id: makeLocationId(location), + actualLocation, + condition }); } }; @@ -66,21 +69,11 @@ export function generatePendingBreakpoint(breakpoint) { } export const simpleMockThreadClient = { - setBreakpoint: (location, _condition) => { - return new Promise((resolve, reject) => { - resolve({ id: "hi", actualLocation: location }); - }); - }, + setBreakpoint: (location, _condition) => + Promise.resolve({ id: "hi", actualLocation: location }), - removeBreakpoint: _id => { - return new Promise((resolve, reject) => { - resolve({ status: "done" }); - }); - }, + removeBreakpoint: _id => Promise.resolve({ status: "done" }), - setBreakpointCondition: (_id, _location, _condition, _noSliding) => { - return new Promise((resolve, reject) => { - resolve({ sourceId: "a", line: 5 }); - }); - } + setBreakpointCondition: (_id, _location, _condition, _noSliding) => + Promise.resolve({ sourceId: "a", line: 5 }) }; From 63305a4b624ec2a2a96822812d5f1c384c9d0fcf Mon Sep 17 00:00:00 2001 From: Yulia Startsev Date: Mon, 15 May 2017 14:15:45 +0200 Subject: [PATCH 08/11] make flow type checking happier --- src/actions/tests/pending-breakpoints.js | 16 ++++++++-------- src/reducers/breakpoints.js | 9 +++++---- src/types.js | 1 + 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/actions/tests/pending-breakpoints.js b/src/actions/tests/pending-breakpoints.js index 3feee4fc7e..2f4582633a 100644 --- a/src/actions/tests/pending-breakpoints.js +++ b/src/actions/tests/pending-breakpoints.js @@ -49,14 +49,14 @@ describe("when adding breakpoints", () => { describe("adding and deleting breakpoints", () => { let breakpoint1; let breakpoint2; - let breakpointId1; - let breakpointId2; + let breakpointLocationId1; + let breakpointLocationId2; beforeEach(() => { breakpoint1 = generateBreakpoint("foo"); breakpoint2 = generateBreakpoint("foo2"); - breakpointId1 = makePendingLocationId(breakpoint1.location); - breakpointId2 = makePendingLocationId(breakpoint2.location); + breakpointLocationId1 = makePendingLocationId(breakpoint1.location); + breakpointLocationId2 = makePendingLocationId(breakpoint2.location); }); it("add a corresponding pendingBreakpoint for each addition", async () => { @@ -65,10 +65,10 @@ describe("when adding breakpoints", () => { await dispatch(actions.addBreakpoint(breakpoint2.location)); const pendingBps = selectors.getPendingBreakpoints(getState()); - expect(pendingBps.get(breakpointId1)).to.eql( + expect(pendingBps.get(breakpointLocationId1)).to.eql( generatePendingBreakpoint(breakpoint1) ); - expect(pendingBps.get(breakpointId2)).to.eql( + expect(pendingBps.get(breakpointLocationId2)).to.eql( generatePendingBreakpoint(breakpoint2) ); }); @@ -80,8 +80,8 @@ describe("when adding breakpoints", () => { await dispatch(actions.removeBreakpoint(breakpoint1.location)); const pendingBps = selectors.getPendingBreakpoints(getState()); - expect(pendingBps.has(breakpointId1)).not.to.be(true); - expect(pendingBps.has(breakpointId2)).to.be(true); + expect(pendingBps.has(breakpointLocationId1)).not.to.be(true); + expect(pendingBps.has(breakpointLocationId2)).to.be(true); }); }); }); diff --git a/src/reducers/breakpoints.js b/src/reducers/breakpoints.js index b87ae3efbc..7854aa06a9 100644 --- a/src/reducers/breakpoints.js +++ b/src/reducers/breakpoints.js @@ -57,9 +57,10 @@ export function makeLocationId(location: Location) { } export function makePendingLocationId(location: Location) { - let { sourceUrl, line, column } = location; - column = column || ""; - return `${sourceUrl}:${line}:${column}`; + const { sourceUrl, line, column } = location; + const sourceUrlString = sourceUrl || ""; + const columnString = column || ""; + return `${sourceUrlString}:${line}:${columnString}`; } function allBreakpointsDisabled(state) { @@ -241,7 +242,7 @@ export function makePendingBreakpoint(bp: any) { return { condition, disabled, location }; } -function setPendingBreakpoints(state) { +function setPendingBreakpoints(state: any) { prefs.pendingBreakpoints = state.pendingBreakpoints; } diff --git a/src/types.js b/src/types.js index dc9faccfd0..664e490afa 100644 --- a/src/types.js +++ b/src/types.js @@ -30,6 +30,7 @@ export type Mode = export type { Breakpoint, + PendingBreakpoint, Frame, Grip, LoadedObject, From 649843b1d0067eea4a3a48b3b94629616d83317b Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Mon, 15 May 2017 10:57:45 -0400 Subject: [PATCH 09/11] bump ci node --- circle.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/circle.yml b/circle.yml index 399e55a4cd..c8754f3348 100644 --- a/circle.yml +++ b/circle.yml @@ -1,6 +1,6 @@ machine: node: - version: 7.0 + version: 7.7.2 services: - docker environment: From ddfdbe16e4be8658d3a663fa0f4565bdbd9e2a86 Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Mon, 15 May 2017 11:19:45 -0400 Subject: [PATCH 10/11] run jest serially --- circle.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/circle.yml b/circle.yml index c8754f3348..d545af95e9 100644 --- a/circle.yml +++ b/circle.yml @@ -20,7 +20,7 @@ test: - mkdir -p $CIRCLE_TEST_REPORTS/mocha - mkdir -p $CIRCLE_TEST_REPORTS/flow-coverage - mkdir -p $CIRCLE_TEST_REPORTS/jest-coverage - - node ./node_modules/.bin/jest --testResultsProcessor jest-junit-reporter --coverage --coverageDirectory=$CIRCLE_TEST_REPORTS/jest-coverage + - jest -i --testResultsProcessor jest-junit-reporter --coverage --coverageDirectory=$CIRCLE_TEST_REPORTS/jest-coverage - yarn license-check - yarn lint-css - yarn lint-js From a1609f355f85a8822757d40101f1e086429c27e4 Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Mon, 15 May 2017 12:21:39 -0400 Subject: [PATCH 11/11] fix mochitests --- src/actions/sources.js | 4 +-- src/test/mochitest/browser_dbg-sources.js | 38 ++++++++++++++--------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/actions/sources.js b/src/actions/sources.js index 5f91f72442..87ebcc9db6 100644 --- a/src/actions/sources.js +++ b/src/actions/sources.js @@ -88,12 +88,12 @@ async function checkPendingBreakpoints(state, dispatch, source) { */ export function newSource(source: Source) { return async ({ dispatch, getState }: ThunkArgs) => { + dispatch({ type: constants.ADD_SOURCE, source }); + if (prefs.clientSourceMapsEnabled) { await dispatch(loadSourceMap(source)); } - dispatch({ type: constants.ADD_SOURCE, source }); - checkSelectedSource(getState(), dispatch, source); await checkPendingBreakpoints(getState(), dispatch, source); }; diff --git a/src/test/mochitest/browser_dbg-sources.js b/src/test/mochitest/browser_dbg-sources.js index 39b6ed6c82..ed70bd86f1 100644 --- a/src/test/mochitest/browser_dbg-sources.js +++ b/src/test/mochitest/browser_dbg-sources.js @@ -11,11 +11,11 @@ function* waitForSourceCount(dbg, i) { }); } -add_task(function* () { +add_task(function*() { const dbg = yield initDebugger("doc-sources.html"); const { selectors: { getSelectedSource }, getState } = dbg; - yield waitForSources(dbg, "simple1"); + yield waitForSources(dbg, "simple1", "simple2", "nested-source", "long.js"); // Expand nodes and make sure more sources appear. is(findAllElements(dbg, "sourceNodes").length, 2); @@ -27,15 +27,21 @@ add_task(function* () { is(findAllElements(dbg, "sourceNodes").length, 8); // Select a source. - ok(!findElementWithSelector(dbg, ".sources-list .focused"), - "Source is not focused"); + ok( + !findElementWithSelector(dbg, ".sources-list .focused"), + "Source is not focused" + ); const selected = waitForDispatch(dbg, "SELECT_SOURCE"); clickElement(dbg, "sourceNode", 4); yield selected; - ok(findElementWithSelector(dbg, ".sources-list .focused"), - "Source is focused"); - ok(getSelectedSource(getState()).get("url").includes("nested-source.js"), - "The right source is selected"); + ok( + findElementWithSelector(dbg, ".sources-list .focused"), + "Source is focused" + ); + ok( + getSelectedSource(getState()).get("url").includes("nested-source.js"), + "The right source is selected" + ); // Make sure new sources appear in the list. ContentTask.spawn(gBrowser.selectedBrowser, null, function() { @@ -45,16 +51,20 @@ add_task(function* () { }); yield waitForSourceCount(dbg, 9); - is(findElement(dbg, "sourceNode", 7).textContent, - "math.min.js", - "The dynamic script exists"); + is( + findElement(dbg, "sourceNode", 7).textContent, + "math.min.js", + "The dynamic script exists" + ); // Make sure named eval sources appear in the list. ContentTask.spawn(gBrowser.selectedBrowser, null, function() { content.eval("window.evaledFunc = function() {} //# sourceURL=evaled.js"); }); yield waitForSourceCount(dbg, 11); - is(findElement(dbg, "sourceNode", 2).textContent, - "evaled.js", - "The eval script exists"); + is( + findElement(dbg, "sourceNode", 2).textContent, + "evaled.js", + "The eval script exists" + ); });