Unit tests for the Grid Outline component

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: Inspector
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: micah, Assigned: micah)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
The grid outline component needs unit tests. These tests should especially verify correctness of extreme case usage of the grid outline.
Priority: -- → P3
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).
(Assignee)

Updated

a year ago
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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.
Attachment #8875186 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/830842118f8a
Add units tests for the Grid Outline component. r=gl

Comment 9

11 months ago
Created attachment 8878279 [details] [diff] [review]
1356474.patch
Attachment #8875186 - Attachment is obsolete: true
Attachment #8878279 - Flags: review+

Comment 10

11 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1520300fc7
Add units tests for the Grid Outline component. r=gl
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

11 months ago
Created attachment 8878307 [details] [diff] [review]
1356474.patch
Attachment #8878279 - Attachment is obsolete: true
Attachment #8878307 - Flags: review+

Comment 13

11 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/955e237fc290
Add units tests for the Grid Outline component. r=gl
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.
Flags: needinfo?(tigleym)

Comment 15

11 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9683155991f
Add units tests for the Grid Outline component. r=gl

Comment 16

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e9683155991f
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Comment 17

11 months ago
(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.
Flags: needinfo?(tigleym)

Updated

11 months ago
Duplicate of this bug: 1306557
You need to log in before you can comment on or make changes to this bug.