Closed Bug 1467407 Opened 5 years ago Closed 5 years ago

Fix incorrect usage of ok() in devtools mochitests

Categories

(DevTools :: General, enhancement, P3)

61 Branch
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(3 files)

Comment on attachment 8984077 [details]
Bug 1467407 - Fix incorrect usage of ok() in server animation test;

https://reviewboard.mozilla.org/r/249924/#review256174

Thanks Julian!
Attachment #8984077 - Flags: review?(dakatsuka) → review+
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8984079 [details]
Bug 1467407 - Fix incorrect usage of ok() in devtools mochitests;

https://reviewboard.mozilla.org/r/249928/#review256296

Thanks! :)
Attachment #8984079 - Flags: review?(jryans) → review+
Cf Bug 1389251 comment 30, :valentin will update browser_webconsole_context_menu_copy_entire_message.js, I will remove it from the queue here to avoid merge conflicts.
See Also: → 1389251
Comment on attachment 8984078 [details]
Bug 1467407 - Fix incorrect usage of ok() in devtools keyboard nav mochitest;

https://reviewboard.mozilla.org/r/249926/#review256338

Yes the behaviour of setting focus on the iframe is good. I think Marco (from a11y team) was involved in fixing toolbox keyboard accessibility recently. Please see comments below that will pass the test.

::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:110
(Diff revision 1)
>    await onKeyEvent;
>  
>    let onceSelected = toolbox.once("webconsole-selected");
>    EventUtils.synthesizeKey("Enter");
>    await onceSelected;
> -  ok(doc.activeElement.id, tabButtons[1].id, "Changed tab button is focused.");
> +  todo_is(doc.activeElement.id, tabButtons[1].id, "Changed tab button is focused.");

We can just change the second argument to
"toolbox-panel-iframe-" + toolbox.currentToolId to make it pass.

::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:125
(Diff revision 1)
>  
>    onceSelected = toolbox.once("inspector-selected");
>    EventUtils.synthesizeKey(" ");
>    await onceSelected;
>  
> -  ok(doc.activeElement.id, tabButtons[0].id, "Changed tab button is focused.");
> +  todo_is(doc.activeElement.id, tabButtons[0].id, "Changed tab button is focused.");

We can just change the second argument to
"toolbox-panel-iframe-" + toolbox.currentToolId to make it pass.
Attachment #8984078 - Flags: review?(yzenevich) → review+
Is it possible to write an eslint rule to catch this kind of errors?
(In reply to Masatoshi Kimura [:emk] from comment #9)
> Is it possible to write an eslint rule to catch this kind of errors?

I didn't consider it because ok() can take more than 2 arguments:
 https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/testing/mochitest/browser-test.js#1277-1283

I wrote a rule to check the arguments passed to ok(), it found 66 violations in m-c with ~50% of false positives. Hard for me to say if it's worth adding to our rules, but I will log a follow up to discuss this.
See Also: → 1467712
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b8df05804e2
Fix incorrect usage of ok() in server animation test;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/13714116865d
Fix incorrect usage of ok() in devtools keyboard nav mochitest;r=yzen
https://hg.mozilla.org/integration/autoland/rev/79e6b5e97fed
Fix incorrect usage of ok() in devtools mochitests;r=jryans
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.