Closed Bug 1471764 Opened Last year Closed Last year

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

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Summary: Add unit tests for the toggling the grid highlighter from the display badges → Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges
Assignee: gl → nobody
Status: ASSIGNED → NEW
Assignee: nobody → gl
Status: NEW → ASSIGNED
Summary: Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges → Add unit tests for the toggling the grid highlighter from the markup display badges
Summary: Add unit tests for the toggling the grid highlighter from the markup display badges → Add unit tests for the toggling the flexbox and grid highlighter from the markup display badges
Attached patch 1471764.patch (obsolete) — Splinter Review
Attachment #9005900 - Flags: review?(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.
Attachment #9005923 - Flags: review?(jdescottes) → review+
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.
(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.
Pushed by gabriel.luong@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/d8ff6c1c6f3a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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.
Status: RESOLVED → REOPENED
No longer depends on: 1488450
Flags: needinfo?(gl)
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
Backout by archaeopteryx@coole-files.de:
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
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/3509e0b96bc1
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.