1147751 - Need assertions for numeric comparison

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Current implementation for numeric comparison is like below:

 Assert.ok(index > 0);

and if the assertion fails, failure message is:

 ERROR AssertionError: false == true

It's very hard to know what happened there. We need new assertions for numeric comparison to be more descriptive message in failure case.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add some new assertions

New assertions are:

Assert.greater
Assert.greaterEqual
Assert.less
Assert.lessEqual


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

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8583608 [details] [diff] [review]
Add some new assertions

Shifting review to Mike, who added this file. AIUI this module is intended to implement the CommonJS unit testing standard:
http://wiki.commonjs.org/wiki/Unit_Testing/1.0

So I'm not sure how Mike feels about extending the API.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8583608 [details] [diff] [review]
Add some new assertions

Review of attachment 8583608 [details] [diff] [review]:
-----------------------------------------------------------------

This is already way more than f+, but not quite ready for landing yet, so r-.

We already extended the assertion framework to support Promise assertions, which are incredibly convenient but not in the spec.
In fact, the spec hasn't been updated in a long time and the group behind it can be seen as inactive.

We should feel free to extend this library to the extent of usefulness to us internally. Therefore, I applaud this change! :-)

::: testing/modules/Assert.jsm
@@ +447,5 @@
>    this.report(false, expected, expected, message);
>  };
>  
>  /**
> + * 12. A promise that is expected to reject:

Please revert this change. The numbers are references to the respective sections in the CommonJS spec.

@@ +477,5 @@
>      ).then(null, reject);
>    });
>  };
> +
> +proto._compareNumber = function compareNumber(expression, lhs, rhs, message, operator) {

nit: please rename this to `_compareNumbers`

@@ +488,5 @@
> +  }
> +
> +  let errorMessage;
> +  if (!lhsIsNumber && !rhsIsNumber) {
> +    errorMessage = "Neither '" + lhs + "' nor '" + rhs + "' is not a number";

Nit: please rephrase to `"Neither '" + lhs + "' nor '" + rhs + "' are numbers";`

@@ +493,5 @@
> +  } else {
> +    errorMessage = "'" + (lhsIsNumber ? rhs : lhs) + "' is not a number";
> +  }
> +  this.report(true, lhs, rhs, errorMessage);
> +}

What you _could_ do here to prevent polluting the prototype is the following:

1. Don't add `_compareNumber` to the prototype
2. Keep the `compareNumbers` function in the current scope
3. Call the function as: `compareNumbers.call(this, lhs <= rhs, lhs, message, ">");`

@@ +496,5 @@
> +  this.report(true, lhs, rhs, errorMessage);
> +}
> +
> +/**
> + * 13. The lhs must be greater than the rhs.

Please remove the numbering in front of the comments.

@@ +521,5 @@
> + *        (number) The right-hand side value
> + * @param message (optional)
> + *        (string) Short explanation of the comparison result
> + */
> +proto.greaterEqual = function greaterEqual(lhs, rhs, message) {

I think I'd prefer `greaterOrEqual` here. Cost: verbosity. Win: more like English.

@@ +551,5 @@
> + *        (number) The right-hand side value
> + * @param message (optional)
> + *        (string) Short explanation of the comparison result
> + */
> +proto.lessEqual = function lessEqual(lhs, rhs, message) {

Same as above, but then `lessOrEqual`. In fact, this is easy to be misread as 'lhs is less equal than rhs' (as in hierarchy), instead of 'lhs is less or equal than rhs`.

::: testing/modules/tests/xpcshell/test_assert.js
@@ +305,5 @@
> +  let ns = {};
> +  Components.utils.import("resource://testing-common/Assert.jsm", ns);
> +  let assert = new ns.Assert();
> +
> +  let message;

Please add these tests right below the last test case inside the `run_test()` function, just above `run_next_test()` (line 301)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add numeric comparison assertions v2

mikedeboer, thanks you for reviewing.

(In reply to Mike de Boer [:mikedeboer] from comment #3)
 
> ::: testing/modules/Assert.jsm
> @@ +447,5 @@
> >    this.report(false, expected, expected, message);
> >  };
> >  
> >  /**
> > + * 12. A promise that is expected to reject:
> 
> Please revert this change. The numbers are references to the respective
> sections in the CommonJS spec.

Reverted.

> @@ +477,5 @@
> >      ).then(null, reject);
> >    });
> >  };
> > +
> > +proto._compareNumber = function compareNumber(expression, lhs, rhs, message, operator) {
> 
> nit: please rename this to `_compareNumbers`

Fixed.

> @@ +488,5 @@
> > +  }
> > +
> > +  let errorMessage;
> > +  if (!lhsIsNumber && !rhsIsNumber) {
> > +    errorMessage = "Neither '" + lhs + "' nor '" + rhs + "' is not a number";
> 
> Nit: please rephrase to `"Neither '" + lhs + "' nor '" + rhs + "' are
> numbers";`

Fixed. I really appreciate this kinds of English mistake.

> @@ +493,5 @@
> > +  } else {
> > +    errorMessage = "'" + (lhsIsNumber ? rhs : lhs) + "' is not a number";
> > +  }
> > +  this.report(true, lhs, rhs, errorMessage);
> > +}
> 
> What you _could_ do here to prevent polluting the prototype is the following:
> 
> 1. Don't add `_compareNumber` to the prototype
> 2. Keep the `compareNumbers` function in the current scope
> 3. Call the function as: `compareNumbers.call(this, lhs <= rhs, lhs,
> message, ">");`

Changed all. Looks better!

> @@ +496,5 @@
> > +  this.report(true, lhs, rhs, errorMessage);
> > +}
> > +
> > +/**
> > + * 13. The lhs must be greater than the rhs.
> 
> Please remove the numbering in front of the comments.

Removed.

> @@ +521,5 @@
> > + *        (number) The right-hand side value
> > + * @param message (optional)
> > + *        (string) Short explanation of the comparison result
> > + */
> > +proto.greaterEqual = function greaterEqual(lhs, rhs, message) {
> 
> I think I'd prefer `greaterOrEqual` here. Cost: verbosity. Win: more like
> English.

Changed to `greaterOrEqual`.

> @@ +551,5 @@
> > + *        (number) The right-hand side value
> > + * @param message (optional)
> > + *        (string) Short explanation of the comparison result
> > + */
> > +proto.lessEqual = function lessEqual(lhs, rhs, message) {
> 
> Same as above, but then `lessOrEqual`. In fact, this is easy to be misread
> as 'lhs is less equal than rhs' (as in hierarchy), instead of 'lhs is less
> or equal than rhs`.

Changed all.

FYI, I just picked the previous names from Python.
https://docs.python.org/2.7/library/unittest.html#unittest.TestCase.assertGreaterEqual

> ::: testing/modules/tests/xpcshell/test_assert.js
> @@ +305,5 @@
> > +  let ns = {};
> > +  Components.utils.import("resource://testing-common/Assert.jsm", ns);
> > +  let assert = new ns.Assert();
> > +
> > +  let message;
> 
> Please add these tests right below the last test case inside the
> `run_test()` function, just above `run_next_test()` (line 301)

Fixed.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58170dda2b4e

Thanks you,

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8597089 [details] [diff] [review]
Add numeric comparison assertions v2

Review of attachment 8597089 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ Fantastic!

There's plenty of bikeshedding that we can do around the name of the lessOrEqual/ greaterOrEqual, but let's not :) I like it much better this way and if you're ok with it, we'll keep it!
As an aside, a spec that contains function names like 'assertAlmostEqual' makes me frown a bit, so let's take it with a grain of salt ;-)

Please don't forget to update the commit message to contain the correct function names.

Thanks!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add numeric comparison assertions v3

Carrying over review+.

(In reply to Mike de Boer [:mikedeboer] from comment #5)

> Please don't forget to update the commit message to contain the correct
> function names.

Oh! I'd totally forgotten it! Thanks a lot!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb087ed9f8cd

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb087ed9f8cd