1356474 - Unit tests for the Grid Outline component

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Micah Tigley [:micah]]

The grid outline component needs unit tests. These tests should especially verify correctness of extreme case usage of the grid outline.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

We should take in account the changes made by bug 1348919 here: since we do not have specific test for the `moveInfobar` function, but we're testing the highlighters that are using it. In the grid outline test, we should add some tests about the grid highlighters' infobars too (e.g. checking that they're hidden when offscreen, and they're properly positioned on top / bottom).

Comment 2

[Micah Tigley [:micah]]

Attachment: Bug 1356474 - Add units tests for the Grid Outline component.

Review commit: https://reviewboard.mozilla.org/r/146590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146590/

Comment 3

[Micah Tigley [:micah]]

Comment on attachment 8875186 [details]
Bug 1356474 - Add units tests for the Grid Outline component.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146590/diff/1-2/

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8875186 [details]
Bug 1356474 - Add units tests for the Grid Outline component.

https://reviewboard.mozilla.org/r/146590/#review151348

::: commit-message-1e315:1
(Diff revision 2)
> +Bug 1356474 - Unit tests for Grid Outline. r?gl

Add unit tests for the Grid Outline component.

::: devtools/client/inspector/grids/components/GridOutline.js:345
(Diff revision 2)
>      } = this.state;
>  
>      return showOutline ?
>        dom.svg(
>          {
> +          id: "grid-outline-container",

Change this to "grid-outline"

::: devtools/client/inspector/grids/components/GridOutline.js:362
(Diff revision 2)
>      const { selectedGrid } = this.state;
>  
>      return selectedGrid ?
>        dom.div(
>          {
> +          id: "grid-outline",

I think we should change "grid-outline" to "grid-outline-container" here, and rename the CSS as necessary.

::: devtools/client/inspector/grids/test/browser.ini:28
(Diff revision 2)
>  [browser_grids_grid-list-on-mutation-element-added.js]
>  [browser_grids_grid-list-on-mutation-element-removed.js]
>  [browser_grids_grid-list-toggle-multiple-grids.js]
>  [browser_grids_grid-list-toggle-single-grid.js]
>  [browser_grids_highlighter-setting-rules-grid-toggle.js]
> +[browser_grids_grid-outline-selected-grid.js]

This list needs to be alphabetically ordered. It should follow the order that you would see when you look at the directory list.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-cannot-show-outline.js:6
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that outline does not show when cells are too small to be drawn and that "Cannot

s/outline/grid outline

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-cannot-show-outline.js:46
(Diff revision 2)
> +    state.grids[0].highlighted);
> +  checkbox.click();
> +  yield onHighlighterShown;
> +  yield onCheckboxChange;
> +
> +  outline = doc.getElementById("grid-outline-container");

With the changes mentioned in the grid-outline naming changes, I think we can remove this since we are reassigning the outline anyways.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-cannot-show-outline.js:49
(Diff revision 2)
> +  yield onCheckboxChange;
> +
> +  outline = doc.getElementById("grid-outline-container");
> +  let cannotShowGridOutline = doc.querySelector(".grid-outline-text");
> +
> +  info("Checking the grid outline is not rendered.");

Checking the grid outline is not rendered and an appropriate message is shown.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-cannot-show-outline.js:52
(Diff revision 2)
> +  let cannotShowGridOutline = doc.querySelector(".grid-outline-text");
> +
> +  info("Checking the grid outline is not rendered.");
> +  ok(!outline, "Outline component is shown.");
> +
> +  info("Checking the message 'Cannot shown outline for this grid.' is shown.");

Remove this info since we are rewording the previous info.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:6
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that outline changes when grid areas are highlighted on mouseover.

Tests that the grid area and cell are highlighted when mouseovering a grid area in the grid outline.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:50
(Diff revision 2)
> +    state.grids[0].highlighted);
> +  checkbox.click();
> +  yield onHighlighterShown;
> +  yield onCheckboxChange;
> +
> +  let gridOutline = doc.querySelector("#grid-outline-container");

#grid-outline

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:52
(Diff revision 2)
> +  yield onHighlighterShown;
> +  yield onCheckboxChange;
> +
> +  let gridOutline = doc.querySelector("#grid-outline-container");
> +  let gridCellA = gridOutline.children[0].querySelectorAll(
> +    "[data-grid-id = '0'][data-grid-row = '1'][data-grid-column='1']");

s/ = /=

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:54
(Diff revision 2)
> +
> +  let gridOutline = doc.querySelector("#grid-outline-container");
> +  let gridCellA = gridOutline.children[0].querySelectorAll(
> +    "[data-grid-id = '0'][data-grid-row = '1'][data-grid-column='1']");
> +
> +  info("Scrolling into the view of the #grid-outline-container.");

#grid-outline

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:60
(Diff revision 2)
> +  gridCellA[0].scrollIntoView();
> +
> +  info("Hovering over grid cell A in the grid outline.");
> +  let onCellAHighlight = highlighters.once("grid-highlighter-shown",
> +      (event, nodeFront, options) => {
> +        info("Checking show grid cell / areas options are correct.");

Checking the grid highlighter options for the show grid area and cell parameters.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:67
(Diff revision 2)
> +        const { gridFragmentIndex, rowNumber, columnNumber } = showGridCell;
> +
> +        ok(showGridCell, "Show grid cell options are available.");
> +        ok(showGridArea, "Show grid areas options are available.");
> +
> +        info("Checking params passed to showGridArea options are correct.");

Remove this info with the rewording of the prev. info

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-areas.js:72
(Diff revision 2)
> +        info("Checking params passed to showGridArea options are correct.");
> +        is(gridFragmentIndex, 0, "Should be the first grid fragment index.");
> +        is(rowNumber, 1, "Should be the first grid row.");
> +        is(columnNumber, 1, "Should be the first grid column.");
> +
> +        info("Checking grid area name is correct.");

Remove this info. We only want info() for actions and checks. We usually already have another info messages with the is() and ok() checks to know what exactly we are checking for. For info() involving checks we just want to say what we are checking in general.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-cell.js:6
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that outline changes when grid cell is highlighted on mouseover.

Tests that the grid cell is highlighted when mouseovering the grid outline of a grid cell.

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-highlight-cell.js:42
(Diff revision 2)
> +  yield onHighlighterShown;
> +  yield onCheckboxChange;
> +
> +  let gridOutline = doc.querySelector("#grid-outline-container");
> +  let gridCellA = gridOutline.children[0].querySelectorAll(
> +    "[data-grid-id = '0'][data-grid-row = '1'][data-grid-column='1']");

s/ = /=

::: devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-grid.js:6
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that outline is shown when a grid container is selected.

Tests that the grid outline is shown when a grid container is selected

::: devtools/client/inspector/layout/layout.js:18
(Diff revision 2)
>  
>  const { LocalizationHelper } = require("devtools/shared/l10n");
>  const INSPECTOR_L10N =
>    new LocalizationHelper("devtools/client/locales/inspector.properties");
>  
>  const SHOW_GRID_OUTLINE_PREF = "devtools.gridinspector.showGridOutline";

This needs to be removed as well.

Comment 5

[Micah Tigley [:micah]]

Comment on attachment 8875186 [details]
Bug 1356474 - Add units tests for the Grid Outline component.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146590/diff/2-3/

Comment 6

[Micah Tigley [:micah]]

Comment on attachment 8875186 [details]
Bug 1356474 - Add units tests for the Grid Outline component.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146590/diff/3-4/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/830842118f8a
Add units tests for the Grid Outline component. r=gl

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0156ad061c5d for failing something most of the time, see for example https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=830842118f8a0f636aa298dd27a34918d924f746&filter-failure_classification_id=2

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1356474.patch

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1520300fc7
Add units tests for the Grid Outline component. r=gl

Comment 11

[Phil Ringnalda (:philor)]

More consistent failure mode this time, anyway. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f50a8eb778 for "browser_grids_grid-outline-cannot-show-outline.js | The message 'Cannot show outline for this grid' is displayed." like https://treeherder.mozilla.org/logviewer.html#?job_id=107523714&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=107526665&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=107522810&repo=mozilla-inbound, and just one outlier browser_grids_grid-outline-selected-grid.js failure, https://treeherder.mozilla.org/logviewer.html#?job_id=107530497&repo=mozilla-inbound

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1356474.patch

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/955e237fc290
Add units tests for the Grid Outline component. r=gl

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for frequently failing own test browser_grids_grid-outline-selected-grid.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1ea65170bbf4031fc6ee44a29d47a84afe77d1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=955e237fc290e79eecface60d9b1af4d2abe293b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107583079&repo=mozilla-inbound

[task 2017-06-16T07:08:07.971562Z] 07:08:07     INFO - Entering test bound 
[task 2017-06-16T07:08:07.972174Z] 07:08:07     INFO - Adding a new tab with URL: data:text/html;charset=utf-8,%0A%20%20%3Cstyle%20type%3D'text%2Fcss'%3E%0A%20%20%20%20%23grid%20%7B%0A%20%20%20%20%20%20display%3A%20grid%3B%0A%20%20%20%20%7D%0A%20%20%3C%2Fstyle%3E%0A%20%20%3Cdiv%20id%3D%22grid%22%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cella%22%3ECell%20A%3C%2Fdiv%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cellb%22%3ECell%20B%3C%2Fdiv%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cellc%22%3ECell%20C%3C%2Fdiv%3E%0A%20%20%3C%2Fdiv%3E%0A
[task 2017-06-16T07:08:07.972311Z] 07:08:07     INFO - Tab added and finished loading
[task 2017-06-16T07:08:07.972394Z] 07:08:07     INFO - Opening the inspector
[task 2017-06-16T07:08:07.973910Z] 07:08:07     INFO - Opening the toolbox
[task 2017-06-16T07:08:07.976434Z] 07:08:07     INFO - Buffered messages logged at 07:07:23
[task 2017-06-16T07:08:07.978496Z] 07:08:07     INFO - Toolbox opened and focused
[task 2017-06-16T07:08:07.980583Z] 07:08:07     INFO - Waiting for actor features to be detected
[task 2017-06-16T07:08:07.982404Z] 07:08:07     INFO - Selecting the layoutview sidebar
[task 2017-06-16T07:08:07.983910Z] 07:08:07     INFO - Checking the initial state of the Grid Inspector.
[task 2017-06-16T07:08:07.985481Z] 07:08:07     INFO - TEST-PASS | devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-grid.js | There should be no grid outline shown. - 
[task 2017-06-16T07:08:07.987756Z] 07:08:07     INFO - Toggling ON the CSS grid highlighter from the layout panel.
[task 2017-06-16T07:08:07.989888Z] 07:08:07     INFO - Waiting for state predicate "state =>
[task 2017-06-16T07:08:07.991198Z] 07:08:07     INFO -     state.grids.length == 1 &&
[task 2017-06-16T07:08:07.992642Z] 07:08:07     INFO -     state.grids[0].highlighted"
[task 2017-06-16T07:08:07.994364Z] 07:08:07     INFO - Found state predicate "state =>
[task 2017-06-16T07:08:07.995367Z] 07:08:07     INFO -     state.grids.length == 1 &&
[task 2017-06-16T07:08:07.996815Z] 07:08:07     INFO -     state.grids[0].highlighted"
[task 2017-06-16T07:08:07.998313Z] 07:08:07     INFO - Buffered messages logged at 07:07:24
[task 2017-06-16T07:08:07.999991Z] 07:08:07     INFO - Checking the grid outline is shown.
[task 2017-06-16T07:08:08.001884Z] 07:08:08     INFO - TEST-PASS | devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-grid.js | Grid outline is shown. - 
[task 2017-06-16T07:08:08.003658Z] 07:08:08     INFO - Toggling OFF the CSS grid highlighter from the layout panel.
[task 2017-06-16T07:08:08.005586Z] 07:08:08     INFO - Waiting for state predicate "state =>
[task 2017-06-16T07:08:08.006920Z] 07:08:08     INFO -     state.grids.length == 1 &&
[task 2017-06-16T07:08:08.008095Z] 07:08:08     INFO -     !state.grids[0].highlighted"
[task 2017-06-16T07:08:08.009260Z] 07:08:08     INFO - Found state predicate "state =>
[task 2017-06-16T07:08:08.010065Z] 07:08:08     INFO -     state.grids.length == 1 &&
[task 2017-06-16T07:08:08.010779Z] 07:08:08     INFO -     !state.grids[0].highlighted"
[task 2017-06-16T07:08:08.011845Z] 07:08:08     INFO - Buffered messages finished
[task 2017-06-16T07:08:08.013075Z] 07:08:08     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-grid.js | Test timed out - 

Please fix the issue and submit an updated patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9683155991f
Add units tests for the Grid Outline component. r=gl

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9683155991f

Comment 17

[Micah Tigley [:micah]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #14)
> Sorry, this had to be backed out for frequently failing own test
> browser_grids_grid-outline-selected-grid.js:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 4b1ea65170bbf4031fc6ee44a29d47a84afe77d1
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=955e237fc290e79eecface60d9b1af4d2abe293b&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=107583079&repo=mozilla-
> inbound
> 
> [task 2017-06-16T07:08:07.971562Z] 07:08:07     INFO - Entering test bound 
> [task 2017-06-16T07:08:07.972174Z] 07:08:07     INFO - Adding a new tab with
> URL:
> data:text/html;charset=utf-8,
> %0A%20%20%3Cstyle%20type%3D'text%2Fcss'%3E%0A%20%20%20%20%23grid%20%7B%0A%20%
> 20%20%20%20%20display%3A%20grid%3B%0A%20%20%20%20%7D%0A%20%20%3C%2Fstyle%3E%0
> A%20%20%3Cdiv%20id%3D%22grid%22%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cella%22%3E
> Cell%20A%3C%2Fdiv%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cellb%22%3ECell%20B%3C%2F
> div%3E%0A%20%20%20%20%3Cdiv%20id%3D%22cellc%22%3ECell%20C%3C%2Fdiv%3E%0A%20%2
> 0%3C%2Fdiv%3E%0A
> [task 2017-06-16T07:08:07.972311Z] 07:08:07     INFO - Tab added and
> finished loading
> [task 2017-06-16T07:08:07.972394Z] 07:08:07     INFO - Opening the inspector
> [task 2017-06-16T07:08:07.973910Z] 07:08:07     INFO - Opening the toolbox
> [task 2017-06-16T07:08:07.976434Z] 07:08:07     INFO - Buffered messages
> logged at 07:07:23
> [task 2017-06-16T07:08:07.978496Z] 07:08:07     INFO - Toolbox opened and
> focused
> [task 2017-06-16T07:08:07.980583Z] 07:08:07     INFO - Waiting for actor
> features to be detected
> [task 2017-06-16T07:08:07.982404Z] 07:08:07     INFO - Selecting the
> layoutview sidebar
> [task 2017-06-16T07:08:07.983910Z] 07:08:07     INFO - Checking the initial
> state of the Grid Inspector.
> [task 2017-06-16T07:08:07.985481Z] 07:08:07     INFO - TEST-PASS |
> devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-
> grid.js | There should be no grid outline shown. - 
> [task 2017-06-16T07:08:07.987756Z] 07:08:07     INFO - Toggling ON the CSS
> grid highlighter from the layout panel.
> [task 2017-06-16T07:08:07.989888Z] 07:08:07     INFO - Waiting for state
> predicate "state =>
> [task 2017-06-16T07:08:07.991198Z] 07:08:07     INFO -    
> state.grids.length == 1 &&
> [task 2017-06-16T07:08:07.992642Z] 07:08:07     INFO -    
> state.grids[0].highlighted"
> [task 2017-06-16T07:08:07.994364Z] 07:08:07     INFO - Found state predicate
> "state =>
> [task 2017-06-16T07:08:07.995367Z] 07:08:07     INFO -    
> state.grids.length == 1 &&
> [task 2017-06-16T07:08:07.996815Z] 07:08:07     INFO -    
> state.grids[0].highlighted"
> [task 2017-06-16T07:08:07.998313Z] 07:08:07     INFO - Buffered messages
> logged at 07:07:24
> [task 2017-06-16T07:08:07.999991Z] 07:08:07     INFO - Checking the grid
> outline is shown.
> [task 2017-06-16T07:08:08.001884Z] 07:08:08     INFO - TEST-PASS |
> devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-
> grid.js | Grid outline is shown. - 
> [task 2017-06-16T07:08:08.003658Z] 07:08:08     INFO - Toggling OFF the CSS
> grid highlighter from the layout panel.
> [task 2017-06-16T07:08:08.005586Z] 07:08:08     INFO - Waiting for state
> predicate "state =>
> [task 2017-06-16T07:08:08.006920Z] 07:08:08     INFO -    
> state.grids.length == 1 &&
> [task 2017-06-16T07:08:08.008095Z] 07:08:08     INFO -    
> !state.grids[0].highlighted"
> [task 2017-06-16T07:08:08.009260Z] 07:08:08     INFO - Found state predicate
> "state =>
> [task 2017-06-16T07:08:08.010065Z] 07:08:08     INFO -    
> state.grids.length == 1 &&
> [task 2017-06-16T07:08:08.010779Z] 07:08:08     INFO -    
> !state.grids[0].highlighted"
> [task 2017-06-16T07:08:08.011845Z] 07:08:08     INFO - Buffered messages
> finished
> [task 2017-06-16T07:08:08.013075Z] 07:08:08     INFO - TEST-UNEXPECTED-FAIL
> |
> devtools/client/inspector/grids/test/browser_grids_grid-outline-selected-
> grid.js | Test timed out - 
> 
> Please fix the issue and submit an updated patch.

Has been fixed and resolved in gl 's latest patch.