Closed Bug 1458235 Opened 4 years ago Closed 4 years ago

Assert.** should fail if the message argument is defined but not a string

Categories

(Testing :: General, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(3 files)

In bug 1458230, I've found a few cases where we're doing "Assert.ok(foo, 1)" to attempt to check the length of foo.

In general, I think we should check the message argument to all of the Assert methods, and verify that if it is defined, then it is a string.

This would provide at least some level of protection against these types of mistakes.
Depends on: 1458363
Depends on: 1459161
No longer depends on: 1459161
(I'm away today and as you know Monday is a bank holiday, so I likely won't get to this until Tuesday, sorry!)
Comment on attachment 8973172 [details]
Bug 1458235 - Make Assert.* fail if the message argument is defined but not a string.

https://reviewboard.mozilla.org/r/241668/#review248148
Attachment #8973172 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8973172 [details]
Bug 1458235 - Make Assert.* fail if the message argument is defined but not a string.

https://reviewboard.mozilla.org/r/241668/#review248150

::: testing/modules/Assert.jsm:197
(Diff revision 1)
>   *        (string) Operation qualifier used by the assertion method (ex: '==')
>   * @param truncate (optional) [true]
>   *        (boolean) Whether or not `actual` and `expected` should be truncated when printing
>   */
>  proto.report = function(failed, actual, expected, message, operator, truncate = true) {
> +  if (message && typeof message != "string") {

Ah, note that this still won't error if `message === false` (or null, or NaN). Not sure if we should add a check for that or if that's unlikely to happen in this usage pattern. Maybe `Assert.ok(foo, null)` hits that path?
Comment on attachment 8973170 [details]
Bug 1458235 - Fix various cases where Assert.ok or Assert.equal have been called wrongly.

https://reviewboard.mozilla.org/r/241664/#review248152
Attachment #8973170 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8973171 [details]
Bug 1458235 - Drop cases of Components.stack.caller passed as the msg to Assert.* as it isn't necessary as we already work out the stack correctly for these cases.

https://reviewboard.mozilla.org/r/241666/#review248154
Attachment #8973171 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8973172 [details]
Bug 1458235 - Make Assert.* fail if the message argument is defined but not a string.

https://reviewboard.mozilla.org/r/241668/#review248150

> Ah, note that this still won't error if `message === false` (or null, or NaN). Not sure if we should add a check for that or if that's unlikely to happen in this usage pattern. Maybe `Assert.ok(foo, null)` hits that path?

It turns out the extension tests do something weird that ends up passing `null` as message. So I've ended up changing this to `message !== undefined && message !== null && typeof message != "string"`
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9135bdd314c2
Fix various cases where Assert.ok or Assert.equal have been called wrongly. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/03ee745ee8e2
Drop cases of Components.stack.caller passed as the msg to Assert.* as it isn't necessary as we already work out the stack correctly for these cases. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/83fb6c53e740
Make Assert.* fail if the message argument is defined but not a string. r=Gijs
You need to log in before you can comment on or make changes to this bug.