Allow dates for isBelow and isAbove assertions #990

v1adko · 2017-06-06T13:20:58Z

Hi guys.

This closes #980. I took #692 as an "inspiration" and extended the code to support dates as discussed in the issue.

I have encountered some questions along the way and left comments (off course will delete them once we decided what to do at those moments) - I will expand on them within GitHub, to make them easier to notice. So it's a WIP for now, just need a bit of your guidance.

This is my first attempt at actually PR-ing anything open-source, so sorry for mistakes.
I'd really like to contribute :)

Also need to fix up Phantom tests, but wanted to get some feedback and ask these questions first, to understand the general direction and the scope of this PR better.

v1adko · 2017-06-06T13:24:29Z

I have probably made a newbie mistake deleting /chai.js. I'm not sure how exactly that happened, the file is in gitignore... I'd blame 'make clean`, but something definitely went wrong.

v1adko · 2017-06-06T13:25:52Z

lib/chai/core/assertions.js


    if (doLength) {
      new Assertion(obj, msg).to.have.property('length');
    } else {
-      new Assertion(obj, msg).is.a('number');
+      // Since utils/expectTypes is missing from 4.x.x and no better way to assert multiple types


I could not find a better way to assert multiple types, on the original PR expectTypes was suggested, but it's no longer present in the utils

v1adko · 2017-06-06T13:26:41Z

lib/chai/core/assertions.js

@@ -587,6 +606,7 @@ module.exports = function (chai, _) {
   * @api public
   */

+  // Should this also be updated?


Originally we discussed only isAbove and isBelow methods to support dates, but this seems connected as well. Same question goes for assertMost and within.

v1adko · 2017-06-06T13:29:11Z

test/assert.js

+
+    err(function() {
+      assert.isAbove(oneSecondAgo, now);
+    }, 'expected ' + now.toUTCString() + ' to be above ' + oneSecondAgo.toString()); // TODO: Fix different date formatting


One thing I noticed is that date formatting is not uniform. While the first argument gets printed inside the error as a UTC string, second one is not. Probably should not be this way.

The error message is created by util.getMessage inside the Assertion.prototype.assert here.

The util.getMessage uses util.objectDisplay which ends up calling util.inspect that for Dates will call toUTCString() 😄

But the .above | .below assertions are just concatenating the expected value to the message which just calls .toString().

In order to fix that you need to update the call to this.assert.

Unfortunatelly github does not allow me to comment on unchanged lines so I'll try to explain here.

Basically you need to update this:

this.assert( obj > n , 'expected #{this} to be above ' + n , 'expected #{this} to be at most ' + n );

To this:

this.assert( obj > n , 'expected #{this} to be above #{exp}' , 'expected #{this} to be at most #{exp}' , n );

I believe you will need to do the same thing for the .below assertion too

Please let me know if you have any further questions 😄

keithamus · 2017-06-06T17:45:51Z

@v1adko thanks for this!

One general note, before I review this; could you please change the base branch to master. We need to delete the 4.x.x branch but haven't gotten round to it yet!

lucasfcosta

Hi @v1adko, thank you for this PR!

I was thinking if we should actually call these isBefore and isAfter to make it more semantic.

Maybe an alias would be enough, but I'd prefer separing those two into separate assertions.

What do you think?

vieiralucas

Hello @v1adko, thank you so much for putting your time into this.

I think that implementing this as isBefore and isAfter looks cleaner and simpler.

The "assert multiple types" problem goes away too.

meeber · 2017-06-06T22:13:03Z

@lucasfcosta @vieiralucas #980 contains a discussion regarding overloading isBelow and isAbove versus creating separate assertions. As noted in that thread, my preference is to overload in this case (with the stipulation that both the target and given value must both be the same type), and perhaps open a separate PR to alias isBefore and isAfter to them. I think that offers sufficient protection against making a mistake with types, while respecting the notion that "before" and "below" can be used interchangeably when speaking of numbers (and when thinking of dates in numeric fashion), as can "above" and "after".

vieiralucas · 2017-06-06T22:22:34Z

@meeber Having the target and given value must both be the same type looks enough to me

vieiralucas · 2017-06-06T22:30:53Z

lib/chai/core/assertions.js

+        ? 'number'
+        : (objType === 'date') 
+          ? 'date' 
+          : 'number or date';


Same as above, also maybe you could extract this code into a function to avoid duplication.

Where would be a better place for a function like this? Utils?

vieiralucas · 2017-06-07T01:27:20Z

lib/chai/core/assertions.js

+        ? 'number'
+        : (objType === 'date') 
+          ? 'date' 
+          : 'number or date';


I would rather use if | else if | else

What do you think?

Sure, no problem :) I know that there are plans to introduce a linter, would definitely be beneficial for such cases, no more "nits".
Also, you guys can take a look at prettier as an alternative to standard linter.

vieiralucas · 2017-06-07T02:04:40Z

test/assert.js

+
+    err(function() {
+      assert.isAbove(oneSecondAgo, now);
+    }, 'expected ' + now.toUTCString() + ' to be above ' + oneSecondAgo.toString()); // TODO: Fix different date formatting


The error message is created by util.getMessage inside the Assertion.prototype.assert here.

The util.getMessage uses util.objectDisplay which ends up calling util.inspect that for Dates will call toUTCString() 😄

But the .above | .below assertions are just concatenating the expected value to the message which just calls .toString().

In order to fix that you need to update the call to this.assert.

Unfortunatelly github does not allow me to comment on unchanged lines so I'll try to explain here.

Basically you need to update this:

this.assert( obj > n , 'expected #{this} to be above ' + n , 'expected #{this} to be at most ' + n );

To this:

this.assert( obj > n , 'expected #{this} to be above #{exp}' , 'expected #{this} to be at most #{exp}' , n );

I believe you will need to do the same thing for the .below assertion too

Please let me know if you have any further questions 😄

v1adko · 2017-06-07T09:53:43Z

lib/chai/core/assertions.js

@@ -1151,6 +1183,7 @@ module.exports = function (chai, _) {
   * @api public
   */

+  // Should this also be updated? + assertMost and within


Originally we discussed only isAbove and isBelow methods to support dates, but assertLeast seems connected as well. Same question goes for assertMost and within. [restored]

Yup all of those should be updated as well.

v1adko · 2017-06-07T09:59:24Z

lib/chai/core/assertions.js

-    if (typeof n !== 'number') {
-      flagMsg = flagMsg ? flagMsg + ': ' : '';
+    if (!(nType === 'number' || nType === 'date')) {
+      var shoudBeType = (objType === 'number' || objType === 'date' )


@vieiralucas Instead of doing else-if I got rid of the nested ternary. If you think that it contradicts a style or obscures code, let me now and I'll change this too.

Restoring the question - if I were to separate this logic elsewhere, what's the best place? Utils?

BTW, these functions (isAbove and isBelow) are very similar, apart from several strings and conditions, so if you really want to make it DRY, these can be separated into a some kind of assertion constructor that would take a predicate and some other params (like the string 'below/above').

I also know that there are plans to introduce a linter, would definitely be beneficial for such cases, no more "nits".
Also, you guys can take a look at prettier as an alternative to standard linter. [restored]

I think this type of utility function could just be local to the assertions file.

I wouldn't be opposed to a refactoring effort to further reduce duplication among these similar functions, as long as it didn't have a big impact on readability. That'd be a discussion for a separate issue/PR though.

After also updating atMost, atLeast and within there is a definite pattern and code duplication, not only in these lines. So, I'm not sure what parts exactly to separate out, I think it's mostly the same code, just different conditions, so easier would be to create a generic assertion and "plug in" the differences.

v1adko · 2017-06-07T10:06:32Z

test/expect.js

+    // Is this right? Fails with the 'implementation frames error' (same as the 4 original ones in master)
+    err(function () {
+      expect(null).to.have.length.above(0, 'blah');
+    }, "Cannot read property 'length' of null");


This test fails, along with 4 tests concerning instance checks (they fail locally on clean master too)
Any ideas?

The .property assertion is ungracefully failing on this line when the target is null. This causes Chai's implementation frames to not be properly removed from the stack trace, which in turn causes this test using the err helper function to fail. I guess this problem needs a separate issue.

v1adko · 2017-06-07T10:06:42Z

@keithamus Changed to the master base.
Also fixed up some of the comments you guys made (like the UTC strings, thanks @vieiralucas)
Still having an issue with deleted chai.js, several questions and one failing test that I'm not sure how to address.

meeber · 2017-06-07T22:48:34Z

@v1adko Thanks for your work on this! A git checkout chai.js should restore the missing file. When the test suite completes successfully, it'll replace chai.js with an updated version. We don't want that until we release a new version, so you'll need to do git checkout chai.js before pushing after a successful test.

v1adko · 2017-06-09T09:56:44Z

lib/chai/core/assertions.js

+      );
+    }
+
+    // Should it check if before <= after?


I noticed that within method does not check weather the two given params make a valid range. Should it? Or it's assumed that it's gonna always throw and the user would see it from the generic error message?

Let's leave it as-is for now. Can always readdress later.

v1adko · 2017-06-09T09:58:13Z

lib/chai/utils/expectTypes.js

@@ -30,10 +30,11 @@ module.exports = function expectTypes(obj, types, ssfi) {
  flagMsg = flagMsg ? flagMsg + ': ' : '';

  obj = flag(obj, 'object');
+  // TODO: Fix - results in undefined when passed date


if obj is a date, after a flag it returns undefined, do we care?)

It looks to me like expectTypes can only be used on assertion objects in order to make sure the target of the assertion (i.e., the value stored in the object flag) is one of the allowed types. It can't be used to check that arguments passed to an assertion are of the correct type; the object flag would be undefined regardless of the argument type. So you're doing the correct thing by checking the argument type manually.

Got it, non-issue then, got confused about the use-case of this method.

v1adko · 2017-06-09T09:59:51Z

test/assert.js

+    }, 'blah: expected ' + now.toUTCString() + ' to be at least ' + oneSecondAfter.toUTCString());
+
+    // These fail with 'implementation frames not properly filtered'
+    err(function() {


Sadly I still don't know how to approach this problem.

Should be fixed in #992

v1adko · 2017-06-09T10:01:43Z

test/assert.js

+    }, 'blah: type mismatch, expected to most value to be a number');
+  });
+
+  // There's no assert.isWithin(n, a, b)?


While should an expect implement have this options, it looks like there's no simple assert.isWithin()
Should we add an alias?

I'd say leave that for a separate PR.

v1adko · 2017-06-09T10:09:49Z

@meeber Updated the other methods + tests too. And checkout chai worked, thanks 👍
Added several comments to discuss. From the previous issues there are only failing tests and a discussion on a best way to make it DRY and refactor.

Also I'm going to be out of town for the weekend without a computer, so, unfortunately, won't be able to work on this. Will gladly pick it up next week.

meeber · 2017-06-10T22:10:10Z

@v1adko: I opened #992 to address the issue that's causing the failing tests due to implementation frames. We'll get that one merged first, and then you can rebase this branch with the change.

v1adko · 2017-06-12T16:07:12Z

@meeber All right, I'll wait till then

cletusw · 2017-06-12T18:00:48Z

If we could get this in a patch update ASAP that'd be great since this fixes an unnecessary breaking change (#980) from 3.x

v1adko · 2017-06-19T09:39:06Z

@meeber, @vieiralucas Hi :) Don't want to rush you guys or anything, just interested in how those PRs are moving along because they look green and it's already been a week. As I understood I need to wait for them to get merged first.

keithamus · 2017-06-20T16:41:06Z

I think I could live with that refactor 😄. Hopefully #585 will eventually get us to a point where this kind of manual type checking isn't necessary any more.

v1adko · 2017-06-21T08:42:23Z

test/expect.js

    }, "blah: expected [ 1, 2, 3 ] to have a length within 5..7");

    err(function () {
      expect(null).to.be.within(0, 1, 'blah');
-    }, "blah: expected null to be a number");
+    }, "blah: the arguments to within must be numbers or dates");


@keithamus This doesn't seem right to me. Technically here both arguments to within are numbers, argument to expect isn't so the message is not very accurate.

Yes this message is a little vague. Can we add an extra if to just throw saying something like "blah: expected null to be a number or date"

v1adko · 2017-06-21T08:52:52Z

@keithamus Updated all methods. I also feel like L1082-L1094 can be extracted into a separate function (like checkNumberOrDate), what's your take on that?
It's gonna reduce duplication in 4 assertion functions (within is custom).
What's your take on this?

UPD: I've managed to take out all of the common logic into one generic function, please take a look here.

keithamus · 2017-06-21T10:04:55Z

Thanks @v1adko. While the simplicity of creating assertions using the helper is definitely somewhere we want to be in future, I think for now having the repetitive yet easily understandable code in the PR as it stands now works well. In addition, #585 grants us the scope for these kinds of optimisations, where we can truly focus on getting chai assertions to be very easy to write.

Let's just fix up the small issue you mentioned above, then get this PR merged!

keithamus · 2017-06-21T10:47:19Z

Perfect! Thanks @v1adko this looks good to me!

@meeber, @lucasfcosta @vieiralucas as you've commented, do you all want to approve before we move this to merge?

vieiralucas

LGTM

Awesome work @v1adko, thank you ❤️

meeber

@v1adko Thanks again for your diligence on this one. I like this a lot, and it should be an easy transition to refactoring a bit later on in a separate PR. I do have a few nitpicks that I'd like to see cleaned up before we merge this.

Also, let's make sure all of these commits get rebased into a single one (either manually or via github) before merging.

meeber · 2017-06-22T16:53:20Z

lib/chai/core/assertions.js

@@ -1024,7 +1024,7 @@ module.exports = function (chai, _) {
  /**
   * ### .above(n[, msg])
   *
-   * Asserts that the target is a number greater than the given number `n`.
+   * Asserts that the target is number or a date greater than the given number or a date `n` respectively.


This doesn't read exactly right to me. I think: "Asserts that the target is a number or a date greater than the given number or date n respectively."

meeber · 2017-06-22T16:54:42Z

lib/chai/core/assertions.js

-   * Asserts that the target is a number greater than or equal to the given
-   * number `n`. However, it's often best to assert that the target is equal to
+   * Asserts that the target is a number or a date greater than or equal to the given
+   * number or a date `n` respectively. However, it's often best to assert that the target is equal to


This doesn't read exactly right to me. I think: "Asserts that the target is a number or a date greater than or equal to the given number or date n respectively."

meeber · 2017-06-22T16:55:31Z

lib/chai/core/assertions.js

@@ -1197,7 +1215,7 @@ module.exports = function (chai, _) {
  /**
   * ### .below(n[, msg])
   *
-   * Asserts that the target is a number less than the given number `n`.
+   * Asserts that the target is a number or a date less than the given number or a date `n` respectively.


This doesn't read exactly right to me. I think: "Asserts that the target is a number or a date less than the given number or date n respectively."

@meeber I don't see how what you wrote is different from what's already there (green).
Or is it supposed to be the same? Sorry, I'm a bit confused. What's would be the suggestions to make it read better?)

@v1adko In almost every case, the suggestion is "given number or date" instead of "given number or a date". In addition to that, the one for .above is also missing an "a" at the start.

@meeber So no articles then?

Articles for the first half (either "the target is a number or a date", or, "the target is a number or date"). But no articles for the second half ("than the given number or date").

meeber · 2017-06-22T16:56:10Z

lib/chai/core/assertions.js

-   * Asserts that the target is a number less than or equal to the given number
-   * `n`. However, it's often best to assert that the target is equal to its
+   * Asserts that the target is a number or a date less than or equal to the given number
+   * or a date `n` respectively. However, it's often best to assert that the target is equal to its


This doesn't read exactly right to me. I think: "Asserts that the target is a number or a date less than or equal to the given number or date n respectively."

meeber · 2017-06-22T16:57:18Z

lib/chai/core/assertions.js

-   * Asserts that the target is a number greater than or equal to the given
-   * number `start`, and less than or equal to the given number `finish`.
+   * Asserts that the target is a number or a date greater than or equal to the given
+   * number or a date `start`, and less than or equal to the given number or a date `finish` respectively.


This doesn't read exactly right to me. I think: "Asserts that the target is a number or a date greater than or equal to the given number or date start, and less than or equal to the given number or date finish respectively."

meeber · 2017-06-22T17:00:43Z

test/assert.js

+      assert.isBelow(1, now, 'blah');
+    }, 'blah: the argument to below must be a number');
+  });
+


Let's add the custom message (blah) testing to the other failure cases here.

meeber · 2017-06-22T17:05:02Z

test/should.js

+      (now).should.not.be.within(now, null, 'blah');
+    }, "blah: the arguments to within must be dates");
+  });
+


Don't need parenthesis around <var name> whenever it's (<var name>).should; still need parenthesis for the values plugged directly, like (0).should

Ok, if I understood correctly, just omit parenthesis and do now.should instead of (now).should?

meeber · 2017-06-22T17:05:12Z

test/should.js

+      ([]).should.have.length.above(now, 'blah');
+    }, "blah: the argument to above must be a number");
+  });
+


Don't need parenthesis around <var name> whenever it's (<var name>).should; still need parenthesis for the values plugged directly, like (0).should

meeber · 2017-06-22T17:05:17Z

test/should.js

+      ('string').should.have.length.below(now, 'blah');
+    }, "blah: the argument to below must be a number");
+  });
+


Don't need parenthesis around <var name> whenever it's (<var name>).should; still need parenthesis for the values plugged directly, like (0).should

meeber · 2017-06-22T17:05:22Z

test/should.js

+      (now).should.not.be.at.most(undefined);
+    }, "the argument to most must be a date");
+  });
+


Don't need parenthesis around <var name> whenever it's (<var name>).should; still need parenthesis for the values plugged directly, like (0).should

v1adko · 2017-06-22T17:37:14Z

@meeber Done. As for the commits - I think it's better to squash them via GitHub.

meeber · 2017-06-22T17:48:58Z

@v1adko The tests for assert.isAbove need that same custom message loving, but otherwise this looks good to go!

v1adko · 2017-06-22T17:52:48Z

@meeber Missed that, thanks :)
Looks like we're waiting for @lucasfcosta to sign off and it's finally done. 🎉

meeber

LGTM! (To whomever merges: remember to squash into single commit!)

keithamus · 2017-06-23T11:54:41Z

🎉 ✨

cletusw · 2017-06-23T15:26:38Z

Awesome! Can we get a release for this? Thanks.

lucasfcosta · 2017-06-23T20:41:32Z

@cletusw Let's see what other people think, but IMO we should.

In order to avoid having to wait for too long for a release like we did last time I think it would be awesome for us to keep releasing new versions with patch and minor changes as soon as possible. This also improves feedback time and we can fix things more quickly.

What do you think?

keithamus · 2017-06-23T22:05:56Z

Eventually we'll hopefully end up adding semantic-release to this repo like the others; in the meantime anyone is welcome to raise a PR of the output of make release, and we can merge & tag it ✨

v1adko commented Jun 6, 2017

View reviewed changes

v1adko changed the base branch from 4.x.x to master June 6, 2017 17:47

v1adko changed the base branch from master to 4.x.x June 6, 2017 18:00

lucasfcosta reviewed Jun 6, 2017

View reviewed changes

vieiralucas reviewed Jun 6, 2017

View reviewed changes

vieiralucas reviewed Jun 7, 2017

View reviewed changes

v1adko changed the base branch from 4.x.x to master June 7, 2017 09:50

v1adko commented Jun 7, 2017

View reviewed changes

v1adko commented Jun 9, 2017

View reviewed changes

v1adko added 3 commits June 20, 2017 13:25

Fixed most tests

e277921

Fix date uniformity (UTC)

1f2b171

Add comment

c3718ea

Update all methods

870a7f3

v1adko commented Jun 21, 2017

View reviewed changes

More explicit error

ccbaf82

keithamus approved these changes Jun 21, 2017

View reviewed changes

shvaikalesh approved these changes Jun 21, 2017

View reviewed changes

vieiralucas approved these changes Jun 22, 2017

View reviewed changes

meeber reviewed Jun 22, 2017

View reviewed changes

v1adko added 3 commits June 22, 2017 20:30

Documentation - articles consistency

fc56c36

Custom message test in assert

5330208

No parenthesis in should

4b5bb3c

Custom message for isAbove

6638ef4

meeber approved these changes Jun 22, 2017

View reviewed changes

keithamus merged commit 3c932e2 into chaijs:master Jun 23, 2017

v1adko deleted the allow-below-dates-assertion branch June 23, 2017 12:12

keithamus mentioned this pull request Jun 26, 2017

chai@4.1.0 #998

Merged

This was referenced Jul 13, 2017

⬆️ chai -> 4.1.0 homezen/hz-test-helpers#55

Merged

⬆️ chai @ ~4.1.0 homezen/redux-variants#165

Merged

renovate bot mentioned this pull request Feb 14, 2018

Update dependency chai to v4 Urigo/tortilla#61

Closed

snyk-bot mentioned this pull request Mar 15, 2020

[Snyk] Upgrade chai from 4.0.2 to 4.2.0 postmanlabs/sails-env-switch#15

Open

Allow dates for isBelow and isAbove assertions #990

Allow dates for isBelow and isAbove assertions #990

Conversation

v1adko commented Jun 6, 2017 • edited

v1adko commented Jun 6, 2017 • edited

Choose a reason for hiding this comment

v1adko Jun 6, 2017 • edited

Choose a reason for hiding this comment

v1adko Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithamus commented Jun 6, 2017

lucasfcosta left a comment

Choose a reason for hiding this comment

vieiralucas left a comment

Choose a reason for hiding this comment

meeber commented Jun 6, 2017

vieiralucas commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko commented Jun 7, 2017

meeber commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko commented Jun 9, 2017

meeber commented Jun 10, 2017

v1adko commented Jun 12, 2017

cletusw commented Jun 12, 2017

v1adko commented Jun 19, 2017 • edited

keithamus commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko commented Jun 21, 2017 • edited

keithamus commented Jun 21, 2017

keithamus commented Jun 21, 2017

vieiralucas left a comment

Choose a reason for hiding this comment

meeber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1adko commented Jun 22, 2017

meeber commented Jun 22, 2017

v1adko commented Jun 6, 2017 •

edited

v1adko commented Jun 6, 2017 •

edited

v1adko Jun 6, 2017 •

edited

v1adko Jun 6, 2017 •

edited

v1adko Jun 7, 2017 •

edited

v1adko Jun 7, 2017 •

edited

v1adko Jun 7, 2017 •

edited

v1adko Jun 7, 2017 •

edited

v1adko commented Jun 19, 2017 •

edited

v1adko commented Jun 21, 2017 •

edited

v1adko commented Jun 22, 2017 •

edited