mguterl / chai-datetime

Matchers for chai to help with common date equality assertions against JavaScript Date objects.
MIT License
45 stars 17 forks source link

Suggestion: Allow text dates to be auto-converted #19

Closed nataliecarey closed 5 years ago

nataliecarey commented 9 years ago

I'm working with a hotel API where I want to TDD arrival & departure dates. I'd like to simplify:

expect(jsonResponse.arrivalDate).to.equalDate(new Date('2017-01-01'));

To:

expect(jsonResponse.arrivalDate).to.equalDate('2017-01-01');

Essentially if chai-datetime is asked to match a string to a date it would wrap it in new Date(str) to allow a short-hand.

I'm happy to raise a PR but wanted to check if it's a desirable feature or if I'm the only lazy one out there.

kayhadrin commented 9 years ago

That sounds like an interesting feature but it'd be also important to provide a secondary date/time format string so that this works for all sorts of locales.

nataliecarey commented 9 years ago

Makes sense. Therefore it could grow to the fully lazy version:

expect(jsonResponse.arrivalDate).to.equalDate('2017-01-01');

And the semi-lazy version:

expect(jsonResponse.arrivalDate).to.equalDate('2017-01-01', 'YYYY-MM-DD');

mguterl commented 9 years ago

@matcarey this is an interesting suggestion and I need to think about it some more. In the past I've used a helper function to avoid some of the verbosity:

expect(jsonResponse.arrivalDate).to.equalDate(d('2017-01-01))
nataliecarey commented 9 years ago

:) I'm obviously more lazy. The reason I've raised the question rather than a Pull Request is because it's a bit of an opinionated feature. If it's still bugging me this afternoon I'll fork and raise a PR.

I can totally understand if there's a desire to be more strict with the typing rather than support laziness - there's pros and cons.

nataliecarey commented 9 years ago

I believe the commit referenced here improves the situation: https://github.com/dorightdigital/chai-datetime/commit/663359343900373d7f9948b856a7870e5104882b

@kayhadrin - I started playing with adding the format and the implementation grew quite clumsy quite quickly. I decided to leave it simple as otherwise it would defeat the point of my laziness. People who care about that probably care enough to generate real date objects.

kayhadrin commented 9 years ago

No issue with that @matcarey. I can see that date format parsing can become a very hairy task - hence the existence of momentjs and the likes.

mguterl commented 9 years ago

Initially I was skeptical, but I'm starting to think that I might add this feature.

I was looking at the different options that the Date constructor accepts and I started to wonder if the argument passed in is not a date, if we should just delegate to the constructor.

expect(jsonResponse.arrivalDate).to.equalDate('2017-01-01')
expect(jsonResponse.arrivalDate).to.equalDate(2017, 1, 1)
expect(jsonResponse.arrivalDate).to.equalDate(1483246800)

There is one case that I'm unsure about and that is passing no arguments, which defaults to the current date and time.

expect(jsonResponse.arrivalDate).to.equalDate()
kayhadrin commented 9 years ago

For the last case expect(jsonResponse.arrivalDate).to.equalDate(), I think that it would be counter-intuitive to allow no argument. To me at least, .to.equalDate() makes me think that I expect to have a Date object - whichever time it is.

On 29 March 2015 at 23:35, Michael Guterl notifications@github.com wrote:

Initially I was skeptical, but I'm starting to think that I might add this feature.

I was looking at the different options that the Date constructor accepts and I started to wonder if the argument passed in is not a date, if we should just delegate to the constructor.

expect(jsonResponse.arrivalDate).to.equalDate('2017-01-01') expect(jsonResponse.arrivalDate).to.equalDate(2017, 1, 1) expect(jsonResponse.arrivalDate).to.equalDate(1483246800)

There is one case that I'm unsure about and that is passing no arguments, which defaults to the current date and time.

expect(jsonResponse.arrivalDate).to.equalDate()

— Reply to this email directly or view it on GitHub https://github.com/gaslight/chai-datetime/issues/19#issuecomment-87404683 .

nataliecarey commented 9 years ago

Hmm, I'm a bit concerned about ambiguity in a few cases - firstly should epoch time be seconds or milliseconds?

expect(jsonResponse.arrivalDate).to.equalDate(1483246800) // must match the Epoch Time (using UTCSeconds?)

The example you've given is seconds but I would have assumed milliseconds. If there's a common expectation one way or another I'd be happy to support it. If we support either one we should document which one is supported to avoid second-guessing. Arguably my implementation supports milliseconds as one can pass in MS instead of the string e.g. new Date(epochMilliseconds) - if this is the desired behaviour I'd want to document it with a test as well as in the README (which I need to update anyway).

In your third example it looks to me as though you've made a typical mistake:

new Date(2017,1,1) // Wed Feb 01 2017 00:00:00 GMT+0000 (GMT)

That's the 1st Feb not the 1st Jan. For this reason I'd be concerned about encouraging the usage of zero-indexed months in tests - it doesn't read as naturally as I'd want from a test. This bug is far too easy to write and far too hard to spot and I'd rather my tests are safe from that :).

I'd consider the final example either ambiguous or dangerous. There's a theoretical race condition with testing against the current time - time can move on by 1 millisecond at an unpredictable part of the execution of the test. I also wouldn't expect that assertion to be made under that name - I'd read it more as 'equal any date'. I'd suggest that trying to second-guess a meaning from zero-parameters is unhelpful for the user and for the library.

My next steps: I'll document the epoch time feature which works by default, if there's an argument to use seconds instead we can discuss it here. We can also continue to discuss the other versions if you think there's merit in them which has been overlooked.

mguterl commented 9 years ago

I made a few typos in my example :(. The goal was to use the arguments that the Date constructor accepts. Basically what is documented here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

I am having second thoughts about this feature. I forgot about months being 0 indexed and the confusion that could cause. I also think using epoch seconds suffers from the same problem, lack of clarity in the test.

I'm okay with using a string and letting new Date handle the parsing, but I'm not interested in going beyond that with locales and such.

If folks want to construct dates in more complicated ways, I would suggest using moment. I'm not sure what would happen if a moment instance were passed into these matchers, but I would be open to supporting that if there's a desire.

nataliecarey commented 9 years ago

Let's find out. When I get a chance I'll check that it's a string before passing it on and raise a pull request.

I'll start a separate thread about moment as it's not related to this thread.

katzenbar commented 9 years ago

Would using a standard format like ISO-8601 allow us to add the feature without allowing too much ambiguity? I have found it easy enough to write and read without having to look things up, but it was super common in the project I was working on.

There are a few variations for different parts, but it should be easy enough to implement. Wikipedia goes over the allowed variations: http://en.wikipedia.org/wiki/ISO_8601

nataliecarey commented 9 years ago

Those would be covered by this update. I'm basically passing the string provided into new Date() for conversion.

katzenbar commented 9 years ago

What I meant was if we restrict users to a limited number of formats, like only allowing ISO-8601, then we can eliminate issues with lack of clarity in tests since it is an easily understood format.

mguterl commented 9 years ago

@matcarey are you finished with the implementation of this feature on https://github.com/dorightdigital/chai-datetime?

If so, would you mind creating a pull request? I plan on cutting a new release in the next couple of days.

nataliecarey commented 9 years ago

Done. https://github.com/gaslight/chai-datetime/pull/23

mguterl commented 5 years ago

Unresolved discussion on the PR from 3 years ago.