Allow dates for isBelow and isAbove assertions - #990

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

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

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 😄

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?

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.

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 😄

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.

@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.

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.

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.

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.

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

Should be fixed in #992

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.

@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"

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."

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."

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").

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."

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."

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

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?

Yup!

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