Closed Bug 1424690 Opened 2 years ago Closed 2 years ago

Message expanding is broken

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

(Keywords: regression, Whiteboard: [newconsole-mvp])

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: -- → P1
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 ?
Attachment #8936221 - Flags: review?(nchevobbe) → review+
Duplicate of this bug: 1425154
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 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
Attachment #8936896 - Flags: review?(nchevobbe) → review-
Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

Marking this obsolete since it's on Honza's patch
Attachment #8936895 - Attachment is obsolete: true
Attachment #8936895 - Flags: review?(nchevobbe)
Marking affected for 59, also, sounds like it is a regression from the duplicate bug 1425154.
(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
Whiteboard: [newconsole-mvp]
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 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: [...],
})
Attachment #8936896 - Flags: review?(nchevobbe) → review+
Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

https://reviewboard.mozilla.org/r/207616/#review214928
Attachment #8936895 - Flags: review?(nchevobbe) → review+
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 on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

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

Addressed the nits
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?
Flags: needinfo?(nchevobbe)
Comment on attachment 8936896 [details]
Bug 1424690 - Enhanced the browser_webconsole_console_group test;

Marking as obsolete since honza's patch landed
Attachment #8936896 - Attachment is obsolete: true
Flags: needinfo?(nchevobbe)
Comment on attachment 8936895 [details]
Bug 1424690 - Fix message expanding;

Marking as obsolete since the test will be added in another bug
Attachment #8936895 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b1e8f652c879
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.