1471764 - Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

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

Attachment: 1471764.patch

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24611022339c23844cebf4413f9be9e468e419de

Comment 3

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

Attachment: 1471764.patch [1.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d418f372c04a86a3c97a80f488899737f98515c

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 9005923 [details] [diff] [review]
1471764.patch [1.0]

Review of attachment 9005923 [details] [diff] [review]:
-----------------------------------------------------------------

The tests look good, thanks Gabriel! 

I am not sure what motivated the refactor from addHighlightersToView to flags.testing?
I found the previous pattern a bit easier to maintain, but up to you :)

::: devtools/client/inspector/flexbox/flexbox.js
@@ +362,5 @@
>      // Fetch the flex items for the given flex container and the flex item NodeFronts.
>      const flexItems = [];
> +    let flexItemFronts;
> +
> +    try {

This method contains 4 try/catch blocks with the exact same comment. Maybe we could wrap most of the method's body in a try catch rather than repeating the pattern?

::: devtools/client/inspector/markup/test/.eslintrc.js
@@ +3,5 @@
>  module.exports = {
>    // Extend from the shared list of defined globals for mochitests.
> +  "extends": "../../../../.eslintrc.mochitests.js",
> +  "globals": {
> +    "waitUntilState": true

We might want to add this to our .eslintrc.mochitests.js ?

::: devtools/client/inspector/test/shared-head.js
@@ +171,5 @@
>   * @return {CssComputedView} the computed view
>   */
>  function selectComputedView(inspector) {
>    inspector.sidebar.select("computedview");
> +  return inspector.getPanel("computedview").computedView;

Was there a strong reason to stop using this pattern? Was "addHighlightersToView()" called too late here? flags.testing is almost never used in the codebase.

Comment 5

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

Comment on attachment 9005923 [details] [diff] [review]
1471764.patch [1.0]

Review of attachment 9005923 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/flexbox/flexbox.js
@@ +362,5 @@
>      // Fetch the flex items for the given flex container and the flex item NodeFronts.
>      const flexItems = [];
> +    let flexItemFronts;
> +
> +    try {

Fixed.

::: devtools/client/inspector/markup/test/.eslintrc.js
@@ +3,5 @@
>  module.exports = {
>    // Extend from the shared list of defined globals for mochitests.
> +  "extends": "../../../../.eslintrc.mochitests.js",
> +  "globals": {
> +    "waitUntilState": true

Fixed.

::: devtools/client/inspector/test/shared-head.js
@@ +171,5 @@
>   * @return {CssComputedView} the computed view
>   */
>  function selectComputedView(inspector) {
>    inspector.sidebar.select("computedview");
> +  return inspector.getPanel("computedview").computedView;

This was recommended by pbro in https://bugzilla.mozilla.org/show_bug.cgi?id=1317102#c16. It was easier to reason the code with the flags.testing compare to figuring out all the entry points in the unit tests when to addHighlightersToView.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)
> Comment on attachment 9005923 [details] [diff] [review]
> 1471764.patch [1.0]
> 
> 
> ::: devtools/client/inspector/test/shared-head.js
> @@ +171,5 @@
> >   * @return {CssComputedView} the computed view
> >   */
> >  function selectComputedView(inspector) {
> >    inspector.sidebar.select("computedview");
> > +  return inspector.getPanel("computedview").computedView;
> 
> This was recommended by pbro in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1317102#c16. It was easier to
> reason the code with the flags.testing compare to figuring out all the entry
> points in the unit tests when to addHighlightersToView.

Thanks for replying. Reading the review from pbro, it looks like the goal is to avoid adding the listeners twice. Makes sense and I guess handling that in addHighlighters... would be non trivial and hard to understand as well.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ff6c1c6f3a
Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges. r=jdescottes

Comment 8

[Daniel Varga [:dvarga]]

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

Comment 9

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

This got backed out for a beta simulation failure, see bug 1488450. It had to be backed out to unblock the last central to beta merge. Feel encouraged to request beta uplift once it has landed on central.

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/bdf475b97f93
Backed out changeset d8ff6c1c6f3a for failing devtools' browser_markup_flex_display_badge.js in beta simulations. a=backout

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3509e0b96bc1
Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges. r=jdescottes

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/3509e0b96bc1