Closed Bug 1147751 Opened 6 years ago Closed 6 years ago

Need assertions for numeric comparison

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Add some new assertions (obsolete) — Splinter Review
New assertions are:

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


https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c7548a9b81
Assignee: nobody → hiikezoe
Attachment #8583608 - Flags: review?(ted)
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.
Attachment #8583608 - Flags: review?(ted) → review?(mdeboer)
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)
Attachment #8583608 - Flags: review?(mdeboer) → review-
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,
Attachment #8583608 - Attachment is obsolete: true
Attachment #8597089 - Flags: review?(mdeboer)
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!
Attachment #8597089 - Flags: review?(mdeboer) → review+
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!
Attachment #8597089 - Attachment is obsolete: true
Attachment #8597103 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb087ed9f8cd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.