Closed Bug 1467712 Opened 2 years ago Closed Last year

Fail tests when ok() is called with more than 2 arguments

Categories

(Testing :: General, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files, 7 obsolete files)

59 bytes, text/x-review-board-request
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Follow up to Bug 1467407.

It seems that relatively often, in mochitests, we use ok() when we intend to use is()

  ok(value, expected, message)

This assert will keep passing as long as `value` is truthy, but the intention of the developer was to check if `value` and `expected` are identical. We could add an eslint rule to check against calls to ok() with more than 2 arguments.

However ok() also _can_ accept more than 2 arguments [1], so this kind of rule might create false positives. I think there are around 30 correct calls to ok() with 3 or more arguments in mochitests right now, and as many incorrect ones.
TBH, I'd prefer to switch ok to Assert.ok (and the same with the rest of the similar functions), as generally they're more explicit and we already have protections in place for those. Unfortunately I've heard not everyone is for that as it is longer...
Comment on attachment 8984372 [details]
Bug 1467712 - add eslint rule to check usage of browser-test ok()

https://reviewboard.mozilla.org/r/250194/#review256496


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/lint/eslint/eslint-plugin-mozilla/lib/configs/browser-test.js:66
(Diff revision 1)
>    "rules": {
>      "mozilla/import-content-task-globals": "error",
>      "mozilla/import-headjs-globals": "error",
>      "mozilla/mark-test-function-used": "error",
> -    "mozilla/no-arbitrary-setTimeout": "error"
> +    "mozilla/max-two-arguments-browser-test-ok": "error",
> +    "mozilla/no-arbitrary-setTimeout": "error",

Error: Unexpected trailing comma. [eslint: comma-dangle]
The other option here would be to consider if we could add a check within the ok function itself, to check that the message is a string, like we did for Assert.*:

https://hg.mozilla.org/mozilla-central/rev/83fb6c53e740
It's difficult to type check this function in its current shape. The three first arguments can be strings:
- condition: normally should be a boolean, but can be a string
- name: normally a string
- ex: can either be an object or a string

We could change the implementation of ok() to expect an optional object as the third parameter. The object could contain "ex" and "stack" properties, to replace the current 3rd and 4th parameters of ok(). And then we could fail if the third parameter is a string.
Julian, would you be interested in taking this further?

I was just talking to Jared about this on irc, and it seems like doing something would be better than nothing.

I think we should do something like this:

- Fail if ok has more than two params. Note that it can be whitelisted.
- Fail completely if Assert.ok has more than two params.

We should also add a test for this, there's existing tests that it can be based on:

https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/tests
Flags: needinfo?(jdescottes)
I am not sure I understand the proposal, sorry :) 

Reading our previous discussion it seems like an option is to:
- always fail ok() if it has more than 2 params
- add another method that supports the original 4 arguments signature with ex and stack ("okWithException" or anything else). I suspect it will mainly be kept to avoid rewriting old tests? If my earlier investigations are correct we only have 30 callsites anyway.

Is this in line with what you propose? Otherwise could you give more details? 

And yes I would be interested in pushing this, I keep getting anxious everytime I need to review tests that rely on "ok()" since I filed this one.
Flags: needinfo?(jdescottes) → needinfo?(standard8)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #9)
> I am not sure I understand the proposal, sorry :) 
> 
> Reading our previous discussion it seems like an option is to:
> - always fail ok() if it has more than 2 params

We should make sure Assert.ok does the same (I think it doesn't at the moment).

> - add another method that supports the original 4 arguments signature with
> ex and stack ("okWithException" or anything else). I suspect it will mainly
> be kept to avoid rewriting old tests? If my earlier investigations are
> correct we only have 30 callsites anyway.

I think that's a good idea to do. It makes it a lot clearer on the expectations.

> And yes I would be interested in pushing this, I keep getting anxious
> everytime I need to review tests that rely on "ok()" since I filed this one.

Cool.
Flags: needinfo?(standard8)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Depends on: 1499096
Attachment #9021221 - Attachment is obsolete: true
Attachment #9021218 - Attachment is obsolete: true
Attachment #9021190 - Attachment is obsolete: true
Depends on D10329
Attachment #9021505 - Attachment is obsolete: true
Attachment #9021431 - Attachment is obsolete: true
Attachment #9021434 - Attachment is obsolete: true
Attachment #9021432 - Attachment is obsolete: true
Attachment #9021637 - Attachment description: Bug 1467712 - Simplify calls to ok to use only 2 arguments → Bug 1467712 - Simplify calls to ok to use only 2 arguments;r=Standard8
Attachment #9021639 - Attachment description: Bug 1467712 - Fail tests if SimpleTest ok() is called with more than 2 arguments;r=Standard8 → Bug 1467712 - Fail if SimpleTest ok() is called with more than 2 arguments;r=Standard8
Attachment #9021638 - Attachment description: Bug 1467712 - Throw if Assert.ok is called with more than 2 arguments;r=Standard8 → Bug 1467712 - Fail if Assert.ok is called with more than 2 arguments;r=Standard8
Summary: Add an eslint rule to check the usage of browser-test ok() → Fail tests when ok() is called with more than 2 arguments
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b19ea76aad6
Simplify calls to ok to use only 2 arguments;r=Standard8
https://hg.mozilla.org/integration/autoland/rev/0088a09d869a
Fail if Assert.ok is called with more than 2 arguments;r=Standard8
https://hg.mozilla.org/integration/autoland/rev/026eb1f6dc6e
Fail if SimpleTest ok() is called with more than 2 arguments;r=Standard8
https://hg.mozilla.org/integration/autoland/rev/9ce0ac2b9d71
Update all test wrappers forwarding to ok;r=Standard8
Moving to testing/general as we ended up not using ESLint for this.

Thank you Julian for fixing this :-)
Component: Lint and Formatting → General
Product: Firefox Build System → Testing
Backed out for multiple failures with Too many arguments passed to ok.

backout: https://hg.mozilla.org/integration/autoland/rev/c1cf77cddadeb002dfe09674ce1d23aa24770010

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ce0ac2b9d71d7e547f07cb6bc4cbbed81ad7a96&group_state=expanded

e.g. failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=209150759&repo=autoland&lineNumber=1287

05:55:49     INFO - TEST-PASS | browser/base/content/test/forms/browser_selectpopup.js | scroll position at drag down from option - 
05:55:49     INFO - Buffered messages finished
05:55:49     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/forms/browser_selectpopup.js | 656 - Too many arguments passed to ok(condition, name)`.
05:55:49     INFO - Stack trace:
05:55:49     INFO - chrome://mochikit/content/browser-test.js:test_ok:1295
05:55:49     INFO - chrome://mochitests/content/browser/browser/base/content/test/forms/browser_selectpopup.js:performLargePopupTests:502
05:55:49     INFO - chrome://mochitests/content/browser/browser/base/content/test/forms/browser_selectpopup.js:test_large_popup:586
05:55:49     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1098
05:55:49     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1089
05:55:49     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:987
05:55:49     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
05:55:49     INFO - TEST-PASS | browser/base/content/test/forms/browser_selectpopup.js | scroll position at mouseup from option should not change -
Flags: needinfo?(jdescottes)
Of course we have some tests that were skipped on linux, will add a 5th patch to fix them
Flags: needinfo?(jdescottes)
Blocks: 1503862
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d60b79922ea
Simplify calls to ok to use only 2 arguments;r=Standard8
https://hg.mozilla.org/integration/autoland/rev/7a72edaf35d7
Fail if Assert.ok is called with more than 2 arguments;r=Standard8
https://hg.mozilla.org/integration/autoland/rev/c9de845be184
Fail if SimpleTest ok() is called with more than 2 arguments;r=Standard8
https://hg.mozilla.org/integration/autoland/rev/4501f17df710
Update all test wrappers forwarding to ok;r=Standard8
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21e6a09e836e
Fix leftover calls to ok() with 3 args in ipc and indexedDB;r=test-only
Depends on: 1505553
Depends on: 1506134
Blocks: 1569235
Depends on: 1586046
No longer depends on: 1586046
You need to log in before you can comment on or make changes to this bug.