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

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Julian Descottes [:jdescottes]]

[1] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/testing/mochitest/browser-test.js#1277-1283

Comment 2

[Mark Banner (:standard8)]

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 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - add eslint rule to check usage of browser-test ok()

Review commit: https://reviewboard.mozilla.org/r/250194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250194/

Comment 4

[Code Review Bot [:reviewbot]]

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]

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Julian Descottes [:jdescottes]]

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.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8984372 [details]
Bug 1467712 - add eslint rule to check usage of browser-test ok()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250194/diff/1-2/

Comment 8

[Mark Banner (:standard8)]

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

Comment 9

[Julian Descottes [: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.

Comment 10

[Mark Banner (: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.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Tests that need okWithException;r=Standard8

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Update test utils to only support 2 arguments;r=Standard8

Depends on D10230

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Update all test wrappers forwarding to ok;r=Standard8

Depends on D10244

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Update test utils to only support 2 arguments;r=Standard8

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Tests that need okWithException;r=Standard8

Depends on D10329

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Update all test wrappers forwarding to ok;r=Standard8

Depends on D10330

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Simplify calls to ok to use only 2 arguments

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Simplify calls to ok to use only 2 arguments;r=Standard8

Comment 19

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Fail if Assert.ok is called with more than 2 arguments;r=Standard8

Depends on D10416

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Fail if SimpleTest ok() is called with more than 2 arguments;r=Standard8

Depends on D10417

Comment 21

[Julian Descottes [:jdescottes]]

Attachment: Bug 1467712 - Update all test wrappers forwarding to ok;r=Standard8

Depends on D10418

Comment 22

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa776d469f4d766f9520ac23ffacdb078469b751

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Mark Banner (:standard8)]

Moving to testing/general as we ended up not using ESLint for this.

Thank you Julian for fixing this :-)

Comment 25

[Natalia Csoregi [:nataliaCs]]

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 -

Comment 26

[Julian Descottes [:jdescottes]]

Of course we have some tests that were skipped on linux, will add a 5th patch to fix them

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21e6a09e836e
Fix leftover calls to ok() with 3 args in ipc and indexedDB;r=test-only

Comment 29

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2d60b79922ea
https://hg.mozilla.org/mozilla-central/rev/7a72edaf35d7
https://hg.mozilla.org/mozilla-central/rev/c9de845be184
https://hg.mozilla.org/mozilla-central/rev/4501f17df710
https://hg.mozilla.org/mozilla-central/rev/21e6a09e836e