Closed Bug 1408949 Opened 7 years ago Closed 7 years ago

Enable browser_webconsole_split.js in the new console frontend

Categories

(DevTools :: Console, defect, P1)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: bgrins)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(3 files)

Bug 1408949 - Convert browser_webconsole_split.js for the new console frontend;
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1408949 - Remove unnecessary clearing of devtools.toolbox.splitconsoleEnabled;
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1408949 - Always clear splitconsole height pref after each test;
59 bytes, text/x-review-board-request
Honza
: review+
Details 
      No description provided.
Blocks: 1400847
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8930957 [details]
Bug 1408949 - Convert browser_webconsole_split.js for the new console frontend;

https://reviewboard.mozilla.org/r/202044/#review207870

Thanks for working on the test Brian!

Please see my inline comments.

Honza

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js:17
(Diff revision 1)
> -  requestLongerTimeout(2);
> +requestLongerTimeout(2);
>  
> -  let {Toolbox} = require("devtools/client/framework/toolbox");
> +add_task(async function () {
>    let toolbox;
>  
> -  loadTab(TEST_URI).then(testConsoleLoadOnDifferentPanel);
> +  await addTab(TEST_URI)

nit: missing semicolon at the and of the row

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js:25
(Diff revision 1)
> +  await checkAllTools();
>  
> -  function testConsoleLoadOnDifferentPanel() {
> -    info("About to check console loads even when non-webconsole panel is open");
> +  info("Testing host types");
> +  checkHostType(Toolbox.HostType.BOTTOM);
> +  checkToolboxUI();
> +  await toolbox.switchHost(Toolbox.HostType.SIDE)

nit: missing semicolon

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js:178
(Diff revision 1)
>         "Deck has a height > 0 when console is split");
>      ok(currentUIState.webconsoleHeight > 0,
>         "Web console has a height > 0 when console is split");
>      is(Math.round(currentUIState.deckHeight + currentUIState.webconsoleHeight),
>         currentUIState.containerHeight,
>         "Everything adds up to container height");

I am seeing test-failure here when running the test in Win10

240 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js | Everything adds up to container height - Got 252, expected 252.067


Should we Math.round both expected/actual values?

I am also seeing that Try+Linux works well. Could be useful to also see Try+Win
Attachment #8930957 - Flags: review?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> devtools/client/webconsole/new-console-output/test/mochitest/
> browser_webconsole_split.js:178
> (Diff revision 1)
> >         "Deck has a height > 0 when console is split");
> >      ok(currentUIState.webconsoleHeight > 0,
> >         "Web console has a height > 0 when console is split");
> >      is(Math.round(currentUIState.deckHeight + currentUIState.webconsoleHeight),
> >         currentUIState.containerHeight,
> >         "Everything adds up to container height");
> 
> I am seeing test-failure here when running the test in Win10
> 
> 240 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/webconsole/new-console-output/test/mochitest/
> browser_webconsole_split.js | Everything adds up to container height - Got
> 252, expected 252.067
> 
> 
> Should we Math.round both expected/actual values?
> 
> I am also seeing that Try+Linux works well. Could be useful to also see
> Try+Win

I tried switching to getBoundingClientRect. Do you still see a failure locally?  It looks OK on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=423fda43ca8a3e4fa6168c6942c806829b1e1d8f.
Comment on attachment 8930957 [details]
Bug 1408949 - Convert browser_webconsole_split.js for the new console frontend;

https://reviewboard.mozilla.org/r/202044/#review209394

Looks good, just couple of inline comments.

It still fails on my machine (Win 10, 64), but I am not seeing that failure on Try,
so it looks like an issue with my configuration..?

Anyway, R+ to not block this, assuming my comments are resolved,

Thanks Brian!

Honza

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js:64
(Diff revision 2)
> -      testBottomHost();
> -    });
>    }
>  
>    function getCurrentUIState() {
>      let win = toolbox.win;

'win' is never used, please remove.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_split.js:195
(Diff revision 2)
>         "The console panel is not the current tool");
>      ok(!currentUIState.buttonSelected, "The command button is not selected.");
> -  });
> +  }
>  
> -  function openPanel(toolId) {
> +  async function openPanel(toolId) {
>      let deferred = defer();

`deferred` is never used, please remove.
Attachment #8930957 - Flags: review?(odvarko) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/879847827de6
Convert browser_webconsole_split.js for the new console frontend;r=Honza
Odd that this isn't the test that was enabled here. My first thought was that a pref wasn't getting unset, but we are already doing that in shared-head: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#120.

Did a try push with the same patch on asan where this failure happened and not seeing the failure after a number of retriggers on dt5: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f198ba96fb0b9c78af81d60dd1d086e7ededd8be
Flags: needinfo?(bgrinstead)
See Also: → 1422179
I can't reproduce the test failure this got backed out for, but when trying to figure it out I did a couple of small cleanups (relying on shared-head to clear user prefs for split console enabled and height) so I figured it's worth submitting them for review anyway
Comment on attachment 8933767 [details]
Bug 1408949 - Remove unnecessary clearing of devtools.toolbox.splitconsoleEnabled;

https://reviewboard.mozilla.org/r/204712/#review210582

Nice cleanup, thanks Brian!

Just one inline question.

Honza

::: devtools/client/webconsole/test/browser_webconsole_split_focus.js
(Diff revision 1)
>    }
>  
>    function finish() {
>      toolbox = TEST_URI = null;
> -    Services.prefs.clearUserPref("devtools.toolbox.splitconsoleEnabled");
> -    Services.prefs.clearUserPref("devtools.toolbox.splitconsoleHeight");

The patch is also removing `splitconsoleHeight`. Is that correct?
Comment on attachment 8933768 [details]
Bug 1408949 - Always clear splitconsole height pref after each test;

https://reviewboard.mozilla.org/r/204714/#review210584

Nice, R+

Pref "devtools.toolbox.splitconsoleHeight" is now auto-cleared too,
so you can ignore my previous question.

Honza
Attachment #8933768 - Flags: review?(odvarko) → review+
Comment on attachment 8933767 [details]
Bug 1408949 - Remove unnecessary clearing of devtools.toolbox.splitconsoleEnabled;

https://reviewboard.mozilla.org/r/204712/#review210586
Attachment #8933767 - Flags: review?(odvarko) → review+
Alright not seeing the error again, going to check in
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/645ca7bf8334
Convert browser_webconsole_split.js for the new console frontend;r=Honza
https://hg.mozilla.org/integration/autoland/rev/7c8899de7edd
Remove unnecessary clearing of devtools.toolbox.splitconsoleEnabled;r=Honza
https://hg.mozilla.org/integration/autoland/rev/52fbbe9e0b84
Always clear splitconsole height pref after each test;r=Honza
https://hg.mozilla.org/mozilla-central/rev/645ca7bf8334
https://hg.mozilla.org/mozilla-central/rev/7c8899de7edd
https://hg.mozilla.org/mozilla-central/rev/52fbbe9e0b84
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: