1424690 - Message expanding is broken

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This is a follow up for bug 1422841

STR:
1) Open the Console panel
2) Log a network request
3) Expand -> OK
4) Collapse doesn't work -> BUG

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1424690 - Fix message expanding;

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936221 [details]
Bug 1424690 - Fix message expanding;

https://reviewboard.mozilla.org/r/206992/#review213316

Thanks honza for the patch, this looks good. Could we have a mocha test for this ?

Comment 4

[Abhinav Koppula]

Attachment: Bug 1424690 - Fix message expanding;

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

Comment 5

[Abhinav Koppula]

Attachment: Bug 1424690 - Enhanced the browser_webconsole_console_group test;

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

Comment 6

[Abhinav Koppula]

Hi Honza,
I talked to Nicolas and have added a small mocha test for the "group toggling" since I thought you people are busy this week with the all-hands.
I hope its fine.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/207618/#review213504

Thanks Abhinav. I have a few comments to make the test more readable.
Also, here we only test the arrow state, could we also check that the store (messagesUi) has the expected ids ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:94
(Diff revision 1)
>    indent = `${indent * INDENT_WIDTH}px`;
>    is(node.querySelector(".indent").style.width, indent,
>      "message has the expected level of indentation");
>  }
> +
> +function testGroupToggle(node, className) {

instead of passing a className, could we only pass an `expanded` boolean ? That would be easier to follow I think

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:96
(Diff revision 1)
>      "message has the expected level of indentation");
>  }
> +
> +function testGroupToggle(node, className) {
> +  let toggleArrow = node.querySelector(".theme-twisty");
> +  let isOpen = className === "startGroup" ? true : false;

we could have it named shouldBeOpen maybe ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:107
(Diff revision 1)
> +function testGroupState(node, isOpen) {
> +  isOpen ? ok(node.classList.contains("open"), `group is not collapsed`) :
> +    ok(!node.classList.contains("open"), `group is collapsed`);
> +}

it's only used in the testGroupToggle and is rather small, so maybe we could inline it, and test directly inside testGroupToggle ? 

like 

```
const isOpen = node => node.classList.contains("open");

is(isOpen(node), shouldBeOpen);
toggleArrow.click();
shouldBeOpen = !shouldBeOpen;

is(isOpen(node), shouldBeOpen);
…
```

i think this should be enough

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

Marking this obsolete since it's on Honza's patch

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 59, also, sounds like it is a regression from the duplicate bug 1425154.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Abhinav Koppula from comment #6)
> Hi Honza,
> I talked to Nicolas and have added a small mocha test for the "group
> toggling" since I thought you people are busy this week with the all-hands.
> I hope its fine.
Yes, thanks!

Honza

Comment 11

[Abhinav Koppula]

Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

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

Comment 12

[Abhinav Koppula]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

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

Comment 13

[Abhinav Koppula]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/207618/#review213504

Hi Nicolas,
I have addressed the comments and have also included the test to check for the expected ids in the store.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/207618/#review214920

I only have a minor comment about testGroupToggle, but with it resolved and a green TRY, this will be good to land.

Thanks Abhinav !

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:42
(Diff revision 2)
> +  testGroupToggle(node, true, store,
> +    ["1","2","3","4","6","8","9","12"], ["1","8","9","12"]);

this is a bit cryptic, could we change testGroupToggle arguments to an object, and call it like : 

testGroupToggle({
  node,
  store,
  shouldBeOpen: true,
  visibleMessageIdsAfterExpand: [...],
  visibleMessageIdsAfterCollapse: [...],
})

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

https://reviewboard.mozilla.org/r/207616/#review214928

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/207618/#review214932

::: commit-message-87ab1:1
(Diff revision 2)
> +Bug 1424690 - Added mocha test for console group toggle; r=nchevobbe

This does not really reflect what the patch does.
It should say that we enhanced the browser_webconsole_console_group test.

Comment 17

[Abhinav Koppula]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

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

Comment 18

[Abhinav Koppula]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/207618/#review214920

Addressed the nits

Comment 19

[Abhinav Koppula]

Hi Nicolas,
TRY shows failures for the test - browser_webconsole_console_group test.
However all tests are passing for webconsole/mochitest on my local even with the --verify parameter.
Can you help me in understanding what might be going wrong?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1e8f652c879
Fix message expanding; r=nchevobbe

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

Marking as obsolete since honza's patch landed

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

Marking as obsolete since the test will be added in another bug

Comment 23

[Eliza Balazs [:ebalazs_]]

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