Closed
Bug 1467407
Opened 6 years ago
Closed 6 years ago
Fix incorrect usage of ok() in devtools mochitests
Categories
(DevTools :: General, enhancement, P3)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
Firefox 62
| Tracking | Status | |
|---|---|---|
| firefox62 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
Attachments
(3 files)
We sometimes use ok() where we should use is(), for instance https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/devtools/client/webconsole/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js#64 Therefore we can miss regression on the impacted tests. The following try push should highlight all the incorrect usage of ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdfe04d0d6d07eb5ec41ccf75283b3f63d0542b9&selectedJob=182232658
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 4•6 years ago
|
||
| mozreview-review | ||
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 | |
Updated•6 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 5•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=004e591bb692f5eade75e8db0b880e9555d0e176
|
||
Comment 6•6 years ago
|
||
| mozreview-review | ||
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+
|
|
Assignee | |
Comment 7•6 years ago
|
||
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 8•6 years ago
|
||
| mozreview-review | ||
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+
|
||
Comment 9•6 years ago
|
||
Is it possible to write an eslint rule to catch this kind of errors?
|
|
Assignee | |
Comment 10•6 years ago
|
||
(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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 15•6 years ago
|
||
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
|
||
Comment 16•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/5b8df05804e2 https://hg.mozilla.org/mozilla-central/rev/13714116865d https://hg.mozilla.org/mozilla-central/rev/79e6b5e97fed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•