Closed Bug 1321238 Opened 3 years ago Closed 3 years ago

Add unit tests for the Grid List and Display Setting component

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1347964
Assignee: nobody → gl
Status: NEW → ASSIGNED
Duplicate of this bug: 1321239
I had to remove the mixin in the GridList component because the grid items did not update on revert for the color picker.
Summary: Add unit tests for the Grid List component → Add unit tests for the Grid List and Display Setting component
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.
Attachment #8872231 - Flags: review?(jdescottes) → review+
Attached patch 1321238.patchSplinter Review
Attachment #8872231 - Attachment is obsolete: true
Attachment #8872417 - Flags: review+
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
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
https://hg.mozilla.org/mozilla-central/rev/2cba4fc3c88a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.