Closed
Bug 1499096
Opened 2 years ago
Closed 2 years ago
Update tests using ok() with 3 or more arguments to use is() when possible
Categories
(Firefox Build System :: Lint and Formatting, enhancement)
Firefox Build System
Lint and Formatting
Tracking
(firefox65 fixed)
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 4 open bugs)
Details
Attachments
(4 files)
Before updating our test helpers to throw when calling ok() with more than 2 arguments, I would like to land a first cleanup that will switch most of the wrong usage of ok() to is() or to other forms of ok(). Normally they should all pass but I want to land this separately and as early as possible in case some of them might be intermittent.
Assignee | ||
Comment 1•2 years ago
|
||
This changeset updates all the test that were wrongly using ok() and wanted to use is() AND for which the assert is still passing without any modification required.
Assignee | ||
Comment 2•2 years ago
|
||
Depends on D8739. This changeset updates calls to ok() that were most likely intended for is(), but are not working as is.
Assignee | ||
Comment 3•2 years ago
|
||
Depends on D8740. This changeset replaces calls to ok with 3 arguments to calls with 2 arguments in situations where the switch does not have a significant impact on the assert.
Assignee | ||
Comment 4•2 years ago
|
||
Depends on D8741 This changeset updates some calls to ok() that should actually be calls to is() and that needed tiny fixes to match the expected value.
Assignee | ||
Updated•2 years ago
|
Summary: Add an eslint rule to check the usage of browser-test ok() → Update tests using ok() with 3 or more arguments to use is() when possible
Assignee | ||
Comment 5•2 years ago
|
||
thanks for the reviews. Pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7f78428027bb457794b71051bf4cd4ff79ce3b1
Comment 6•2 years ago
|
||
Thank you for filing all the bugs.
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eee0effaa1f0 Update tests using ok() to is();r=Standard8 https://hg.mozilla.org/integration/autoland/rev/a82f6a836cf3 Update wrong usage of ok() with todo_is();r=Standard8 https://hg.mozilla.org/integration/autoland/rev/dfaaf3978e3c Use ok() with 2 arguments instead of 3 when possible;r=Standard8 https://hg.mozilla.org/integration/autoland/rev/2e4bdafb70ce Update tests using ok() to is(), with minor fixes;r=Standard8
Comment 8•2 years ago
|
||
Hi Julian, Considering all the implementation bugs you logged as blocking this one: Is the General component the correct one? If not, can you tell me which one would be correct so we can modify it accordingly? This is related to bug triage. Thank you.
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 9•2 years ago
|
||
If we have an general component related to tests we could move them there. Otherwise I can try to triage them individually in the components responsible for the test file impacted by each bug. Let me know what you prefer.
Flags: needinfo?(jdescottes) → needinfo?(daniel.bodea)
Comment 10•2 years ago
|
||
It appears that the component specific for automated tests is (Testing) Marionette. I will mark them as such. Thanks.
Flags: needinfo?(daniel.bodea)
Comment 11•2 years ago
|
||
(In reply to Bodea Daniel [:danibodea] from comment #10) > It appears that the component specific for automated tests is (Testing) > Marionette. I will mark them as such. Thanks. Testing:Marionette is for items related to the marionette test suite. They should be located according to `./mach file-info bugzilla-component <file>`
Flags: needinfo?(daniel.bodea)
Comment 12•2 years ago
|
||
I am sorry for the misunderstanding. Would it be OK if I left them on the General component, but for the "Testing" product?
Flags: needinfo?(daniel.bodea)
Comment 13•2 years ago
|
||
Don't worry, I can put them into the correct places. It is better if they are in the right components as the triage owners can set the priorities correctly as a result.
Comment 14•2 years ago
|
||
Yes, that is good. Thank you.
Comment 15•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eee0effaa1f0 https://hg.mozilla.org/mozilla-central/rev/a82f6a836cf3 https://hg.mozilla.org/mozilla-central/rev/dfaaf3978e3c https://hg.mozilla.org/mozilla-central/rev/2e4bdafb70ce
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 16•2 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #13) > Don't worry, I can put them into the correct places. It is better if they > are in the right components as the triage owners can set the priorities > correctly as a result. You also want to make sure to file them as normal bugs rather than enhancement requests, as the latter aren't necessarily triaged in the same way.
You need to log in
before you can comment on or make changes to this bug.
Description
•