1427006 - Add a test for console grouping by clicking the group icon and check if messages are getting expanded/collapsed

Status: RESOLVED FIXED

(DevTools :: Console, enhancement)

Description

[Abhinav Koppula]

Follow-up of https://bugzilla.mozilla.org/show_bug.cgi?id=1424690
Add a test to verify if message expanding is working properly.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1427006 - Enhanced the browser_webconsole_console_group test;

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

The TRY run on the patch looks good to me. So if you want to abhinav, you can copy what's in there that was missing in your patch, and push to review :)

Comment 3

[Abhinav Koppula]

Attachment: Bug 1427006 - Enhanced the browser_webconsole_console_group test;

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8939925 [details]
Bug 1427006 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/210218/#review216120

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

TRY is fine, let's push

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af69a7e1b59f
Enhanced the browser_webconsole_console_group test; r=nchevobbe

Comment 7

[Tiberius Oros[:tiberius_oros]]

Backed out for failing devtools and test verify tests test/mochitest/browser_webconsole_console_group.js

Backout: https://hg.mozilla.org/integration/autoland/rev/85baf8c8549c227e169462ff732c5b2b7dc48577

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=af69a7e1b59f84302226cd0849f43e2348a30eb1

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=154383846&repo=autoland&lineNumber=2050

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Clearing needinfo since Abhinav is working on a fix for this failure.

Comment 9

[Abhinav Koppula]

Comment on attachment 8939925 [details]
Bug 1427006 - Enhanced the browser_webconsole_console_group test;

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

Comment 10

[Abhinav Koppula]

Hi Nicolas,
I have updated the patch a bit and TRY looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45023344b8072b428c38cbf991612a07b6a3c0c1

Can you also check once from your side? If possible, can we push to autoland and see if there are any failures before actually pushing to mc?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8939925 [details]
Bug 1427006 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/210218/#review218762

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:114
(Diff revision 2)
> +async function isOpen(node) {
> +  return node.classList.contains("open");
> +}

Is there a reason to make this function async ? 
It is only checking a class so it can be non-async I think. But I may miss something ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:118
(Diff revision 2)
> +async function testGroupToggle({ node, store, shouldBeOpen,
> +  visibleMessageIdsAfterExpand, visibleMessageIdsAfterCollapse }) {

nit: turn this into 
```js
async function testGroupToggle({ 
  node, 
  store, 
  shouldBeOpen,
  visibleMessageIdsAfterExpand,     
  visibleMessageIdsAfterCollapse 
}) {
```

so its more readable

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_group.js:133
(Diff revision 2)
> +  await isOpen(node);
> +  assertVisibleMessageIds(shouldBeOpen);
> +
> +  toggleArrow.click();
> +  shouldBeOpen = !shouldBeOpen;
> +  await isOpen(node);

I don't think this is what we want.
Here, unless I miss something, `isOpen` do not need to be async, since we only check a class on a node.

What we want insted, is to wait until the node changed, i.e. : 

```js
await waitFor(() => isOpen(node) === shouldBeOpen)
```

I think what you did fixed some failure on TRY because turning isOpen in a async function wraps everything woth promise, and maybe gives a little more time for the node to be updated, but this is still subject to intermittent failure in my opinion.

Comment 12

[Abhinav Koppula]

Comment on attachment 8939925 [details]
Bug 1427006 - Enhanced the browser_webconsole_console_group test;

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

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8939925 [details]
Bug 1427006 - Enhanced the browser_webconsole_console_group test;

https://reviewboard.mozilla.org/r/210218/#review218900

Looks good with a green TRY push :)

Comment 14

[Abhinav Koppula]

Hi Nicolas,
Pushed to TRY and it looks good to me.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d14ec85dbb410a8daa65b97743fc1025f831e63
Can you also confirm once?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a130721f8ea
Enhanced the browser_webconsole_console_group test; r=nchevobbe

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Thanks Abhinav !

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5a130721f8ea