Since adding the Grid List component in the Grid Inspector. We need to add browser unit tests to verify the contents and test the reducer's methods for UPDATE_GRID_HIGHLIGHTED and UPDATE_GRIDS.

I had to remove the mixin in the GridList component because the grid items did not update on revert for the color picker.

Comment on attachment 8872231 [details] Bug 1321238 - Add unit tests for the Grid List and Grid Display Settings components. https://reviewboard.mozilla.org/r/143708/#review147462 Tests, tests, tests! Have a look at my suggestion to keep the PureRenderMixin + some nits and suggestions. ::: devtools/client/inspector/boxmodel/box-model.js:130 (Diff revision 1) > if (reason) { > this._updateReasons.push(reason); > } > > let lastRequest = Task.spawn((function* () { > - if (!(this.isPanelVisible() && > + if (!this.inspector || !this.isPanelVisible() || nit: this condition being hard to read, move !this.isPanelVisible() on its own line ::: devtools/client/inspector/grids/components/GridItem.js (Diff revision 1) > onSetGridOverlayColor: PropTypes.func.isRequired, > onShowBoxModelHighlighterForNode: PropTypes.func.isRequired, > onToggleGridHighlighter: PropTypes.func.isRequired, > }, > > - mixins: [ addons.PureRenderMixin ], You can keep this, provided we change the UPDATE_GRID_COLOR reducer: > [UPDATE_GRID_COLOR](grids, { nodeFront, color }) { > let newGrids = grids.map(grid => { > if (grid.nodeFront == nodeFront) { > grid = Object.assign({}, grid, { > color > }); > } > > return grid; > }); > > return newGrids; > }, ::: devtools/client/inspector/grids/grid-inspector.js:259 (Diff revision 1) > // Get all the GridFront from the server if no gridFronts were provided. > if (!gridFronts) { > + try { > - gridFronts = yield this.layoutInspector.getAllGrids(this.walker.rootNode); > + gridFronts = yield this.layoutInspector.getAllGrids(this.walker.rootNode); > + } catch(e) { > + return; Add a comment here to explain why the call might fail. ::: devtools/client/inspector/grids/grid-inspector.js:271 (Diff revision 1) > > + let nodeFront; > + try { > + nodeFront = yield this.walker.getNodeFromActor(grid.actorID, ["containerEl"]); > + } catch (e) { > + return; #1 : Same here, comment explaining why the call might fail #2 : We are in a for loop, shouldn't we just continue? If not mention it in the comment too. ::: devtools/client/inspector/grids/test/browser.ini:20 (Diff revision 1) > +[browser_grids_display-setting-show-grid-line-numbers.js] > +[browser_grids_grid-list-color-picker-on-ESC.js] > +[browser_grids_grid-list-color-picker-on-RETURN.js] > +[browser_grids_grid-list-element-rep.js] > +[browser_grids_grid-list-no-grids.js] > +[browser_grids_grid-list-on-mutation_01.js] use descriptive name instead of 01.js "browser_grids_grid-list-on-mutation_element-removed.js" ::: devtools/client/inspector/grids/test/browser.ini:21 (Diff revision 1) > +[browser_grids_grid-list-color-picker-on-ESC.js] > +[browser_grids_grid-list-color-picker-on-RETURN.js] > +[browser_grids_grid-list-element-rep.js] > +[browser_grids_grid-list-no-grids.js] > +[browser_grids_grid-list-on-mutation_01.js] > +[browser_grids_grid-list-on-mutation_02.js] use descriptive name instead of 02.js "browser_grids_grid-list-on-mutation_element-added.js" ::: devtools/client/inspector/grids/test/browser.ini:22 (Diff revision 1) > +[browser_grids_grid-list-color-picker-on-RETURN.js] > +[browser_grids_grid-list-element-rep.js] > +[browser_grids_grid-list-no-grids.js] > +[browser_grids_grid-list-on-mutation_01.js] > +[browser_grids_grid-list-on-mutation_02.js] > +[browser_grids_grid-list-toggle_01.js] ditto ::: devtools/client/inspector/grids/test/browser.ini:23 (Diff revision 1) > +[browser_grids_grid-list-element-rep.js] > +[browser_grids_grid-list-no-grids.js] > +[browser_grids_grid-list-on-mutation_01.js] > +[browser_grids_grid-list-on-mutation_02.js] > +[browser_grids_grid-list-toggle_01.js] > +[browser_grids_grid-list-toggle_02.js] ditto ::: devtools/client/inspector/grids/test/browser_grids_grid-list-color-picker-on-ESC.js:29 (Diff revision 1) > + let { document: doc } = gridInspector; > + let { store } = inspector; > + let cPicker = gridInspector.getSwatchColorPickerTooltip(); > + let spectrum = cPicker.spectrum; > + let swatch = doc.querySelector(".grid-color-swatch"); > + swatch.scrollIntoView(); Should not be silent, an info() would be good ::: devtools/client/inspector/grids/test/browser_grids_grid-list-color-picker-on-ESC.js:30 (Diff revision 1) > + let { store } = inspector; > + let cPicker = gridInspector.getSwatchColorPickerTooltip(); > + let spectrum = cPicker.spectrum; > + let swatch = doc.querySelector(".grid-color-swatch"); > + swatch.scrollIntoView(); > + It would be nice to start the test by checking that the initial state matches our expectations (ie #4B0082 in the state for grids[0].color and bg-color of swatch is rgb(75, 0, 130)). In case the default value changes, this will make the failure easier to understand for someone new to this test. ::: devtools/client/inspector/grids/test/browser_grids_grid-list-color-picker-on-ESC.js:31 (Diff revision 1) > + let cPicker = gridInspector.getSwatchColorPickerTooltip(); > + let spectrum = cPicker.spectrum; > + let swatch = doc.querySelector(".grid-color-swatch"); > + swatch.scrollIntoView(); > + > + let onColorPickerReady = cPicker.once("ready"); info() here ::: devtools/client/inspector/grids/test/browser_grids_grid-list-color-picker-on-RETURN.js:29 (Diff revision 1) > + let { document: doc } = gridInspector; > + let { store } = inspector; > + let cPicker = gridInspector.getSwatchColorPickerTooltip(); > + let spectrum = cPicker.spectrum; > + let swatch = doc.querySelector(".grid-color-swatch"); > + swatch.scrollIntoView(); info() needed ::: devtools/client/inspector/grids/test/browser_grids_grid-list-color-picker-on-RETURN.js:30 (Diff revision 1) > + let { store } = inspector; > + let cPicker = gridInspector.getSwatchColorPickerTooltip(); > + let spectrum = cPicker.spectrum; > + let swatch = doc.querySelector(".grid-color-swatch"); > + swatch.scrollIntoView(); > + test initial state ::: devtools/client/inspector/grids/test/browser_grids_grid-list-color-picker-on-RETURN.js:32 (Diff revision 1) > + let spectrum = cPicker.spectrum; > + let swatch = doc.querySelector(".grid-color-swatch"); > + swatch.scrollIntoView(); > + > + let onColorPickerReady = cPicker.once("ready"); > + swatch.click(); info() needed ::: devtools/client/inspector/grids/test/browser_grids_grid-list-element-rep.js:29 (Diff revision 1) > + let { document: doc } = gridInspector; > + let { store } = inspector; > + > + let gridList = doc.querySelector("#grid-list"); > + let elementRep = gridList.children[0].querySelector(".open-inspector"); > + elementRep.scrollIntoView(); info() needed ::: devtools/client/inspector/grids/test/browser_grids_grid-list-element-rep.js:36 (Diff revision 1) > + info("Listen to node-highlight event and mouse over the widget"); > + let onHighlight = toolbox.once("node-highlight"); > + EventUtils.synthesizeMouse(elementRep, 10, 5, {type: "mouseover"}, doc.defaultView); > + let nodeFront = yield onHighlight; > + > + ok(true, "The node-highlight event was fired"); ok(true, "") -> info("") (or switch to ok(nodeFront, "")?) ::: devtools/client/inspector/grids/test/browser_grids_grid-list-no-grids.js:6 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Tests that no grid list items and a no grids available message is displayed when nit (quotes): a no grids available message -> a 'no grids available' message ::: devtools/client/inspector/grids/test/browser_grids_grid-list-no-grids.js:18 (Diff revision 1) > + <div id="cell1">cell1</div> > + <div id="cell2">cell2</div> > + </div> > +`; > + > +const HIGHLIGHTER_TYPE = "CssGridHighlighter"; Could share this const in head.js and avoid duplicating it in several tests. ::: devtools/client/inspector/grids/test/browser_grids_grid-list-no-grids.js:31 (Diff revision 1) > + yield selectNode("#grid", inspector); > + let noGridList = doc.querySelector(".layout-no-grids"); > + let gridList = doc.querySelector("#grid-list"); > + > + info("Checking the initial state of the Grid Inspector."); > + ok(noGridList, "No grid list displayed."); nit: "The message `no grid containers` is displayed" (the pattern ok(element, "No something displayed") made me pause) ::: devtools/client/inspector/grids/test/browser_grids_grid-list-on-mutation_01.js:64 (Diff revision 1) > + yield onCheckboxChange; > + > + info("Checking the CSS grid highlighter is not shown."); > + ok(!highlighters.gridHighlighterShown, "No CSS grid highlighter is shown."); > + let noGridList = doc.querySelector(".layout-no-grids"); > + ok(noGridList, "No grid list displayed."); nit: "The message `no grid containers` is displayed" ::: devtools/client/inspector/grids/test/browser_grids_grid-list-toggle_01.js:12 (Diff revision 1) > + grid-template-columns: [col-1 col-start-1] 100px [col-2] 100px; > + grid-template-rows: 100px 100px; > + grid-template-areas: ". header" > + "sidebar content"; Is this serving any purpose for this test? ::: devtools/client/inspector/grids/test/browser_grids_grid-list-toggle_01.js:17 (Diff revision 1) > + #cell1 { > + grid-column: 1; > + grid-row: 1; > + } > + #cell2 { > + grid-column: 2; > + grid-row: 1; > + } > + #cell3 { > + grid-column: 1; > + grid-row: 2; > + } > + #cell4 { > + grid-column: 2; > + grid-row: 2; > + } Same question ::: devtools/client/inspector/grids/test/head.js:57 (Diff revision 1) > + }; > + }); > +} > + > +/** > + * * Simulate a color change in a given color picker tooltip. #1 nit: remove the extra "*" #2 add jsdoc #3 For the record. We have 3 implementations of simulateColorChange now; - client/inspector/rules/test/head.js - client/inspector/shared/test/head.js - this one The code you have here is actually the common part done by the two other implementations. We should mutualize this. Can you log a follow up bug? ::: devtools/client/inspector/test/head.js:649 (Diff revision 1) > } > > /** > + * Make sure window is properly focused before sending a key event. > + * @param {Window} win > + * @param {Event} key Event type is wrong. key is a String.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad742747a8d0 Add unit tests for the Grid List and Display setting components. r=jdescottes

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4a50cfa3de80 for eslint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=102876562&repo=mozilla-inbound

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2cba4fc3c88a Add unit tests for the Grid List and Display setting components. r=jdescottes