antonmi / espec

Elixir Behaviour Driven Development
Other
808 stars 62 forks source link

Add Calendar expectation #111

Closed antonmi closed 6 years ago

antonmi commented 8 years ago

Since Elixir 1.3 introduces Calendar module it'd be nice to have corresponding expectations

antonmi commented 8 years ago

Not sure about compatibility with previous Elixir version.

antonmi commented 7 years ago

Support of Elixir 1.2 was removed since ESpec 1.4.0. So it's time to think about Calendar assertions

bozydar commented 7 years ago

What about the following syntax?

expect(~D[2017-01-01]).to be_dated :before, ~D[2017-01-02]
expect(~T[10:00:01]).to be_timed :after, ~T[10:00:00]
expect(~N[2017-01-02 22:00:00]).to be_dated_between ~D[2017-01-01], ~D[2017-01-03]
expect(~N[2017-01-02 22:00:00]).to be_timed_between ~T[21:00:00], ~T[23:00:00]
expect(~N[2017-01-02 22:00:00]).to be_naively_between ~N[2017-01-02 21:00:00], ~N[2017-01-03 21:00:00]
expect(DateTime.from_naive!(~N[2017-01-02 10:00:00], "Etc/UTC")).to be_awarely :after, DateTime.from_naive!(~N[2017-01-01 10:00:00], "Etc/UTC")

and so on...

treble37 commented 7 years ago

I was thinking the easiest way to start tackling this would be to add "be_" specs as called out in the comments at the top of the Calendar module:

For the actual date, time and datetime structures, see `Date`, `Time`, `NaiveDateTime` and `DateTime`.

So I would add Time, NaiveDateTime, etc. as "be_time()"... What do you think @antonmi ? This also wouldn't overlap with bozydar's proposal above.

antonmi commented 7 years ago

Hi, @bozydar and @treble37 ! Thank for your awesome colaboration!

@bozydar I'm not sure that we need so big collection of date/time assertions. I've just experimented with existing equality and comparison matchers and most of them works fine:

it do: expect(~D[2010-04-17]).to be(:==, ~D[2010-04-17])
it do: expect(~D[2010-04-17]).to be(:>, ~D[2010-04-16])
it do: expect(~D[2010-04-17]).to be(:<, ~D[2010-04-18])

it do: expect ~D[2010-04-17] |> to(eq ~D[2010-04-17])
it do: expect ~D[2010-04-17] |> to(be_between ~D[2010-04-14], ~D[2010-04-18])
# this does not work
it do: expect ~D[2010-04-17] |> to(be_close_to ~D[2010-04-17], 1) 

The only problem is be_close_to matcher. be_between seems work incorectly when different structs are used:

it do: expect ~N[2017-01-02 10:00:00] |> to(be_between ~D[2017-01-01], ~D[2017-01-03])

@treble37 I'm not even sure about be_date and etc, because they can be easily replaced by be_struct(Date) and etc.

So my proposal is:

@bozydar , your awesome set of date and time matchers that you proposed in your PR worth to be extracted into a separate library (like 'espec_phoenix_helpers', 'test_that_json_espec', 'bamboo_espec' and etc.)

treble37 commented 7 years ago

ok thank you for the feedback @antonmi, I'll give it a go and see what happens (I'll have more time this weekend I think).

bozydar commented 7 years ago

Hi @antonmi and @treble37 !

I. The problem with the be(symbol, value) is that it compares date/time as they are simple structs what in most cases is ok but not always ("timezoned" DateTime). This is the reason why DateTime.compare/2 was added to Elixir. I was wondering about "overriding" be/2 especially for date and to use Date.compare/2 in these cases when date/time type family appears in args but:

  1. It could be confusing if somebody really wants to compare date structs.
  2. I find it reading easier expect(~N[2017-01-02]).to be_naively :after, ~N[2017-01-01] then expect(~N[2017-01-02]).to be :>, ~N[2017-01-01]. I always need to think what does it mean <, > in case of time/date.

Maybe the solution is to reuse be/2 but with "overriding" and to use DateTime.compare/2 for the cases when :after, :before, etc. symbol appears as the argument. For example: expect(~N[2017-01-02]).to be :after, ~N[2017-01-01].

What do you think guys?

II. I was wondering how to handle to(be_close_to ~D[2010-04-17], 1) case. Probably we need a sub-syntax or helpers to handle time ranges because one doesn't know what this "1" really means. Days, months, years...?

III. Good. I can extract date/time part and build espec_date_time library if you prefer to make it more "pluggable".

treble37 commented 7 years ago

The problem with the be(symbol, value) is that it compares date/time as they are simple structs what in most cases is ok but not always ("timezoned" DateTime). - interesting.

II... I was wondering how to handle to(be_close_to ~D[2010-04-17], 1) case.

I was wondering this too @bozydar - the Date.diff/2 computes in days, so then you could assume the default is days although that might not be apparent readily to the user writing a matcher test such as (be_close_to ~D[2010-04-17], 1). As I think about it, perhaps the default unit should be seconds since that's the smallest possible unit of comparison (we don't want fractional days I assume). Also @antonmi, Date.diff/2 is not available in Elixir 1.3-1.4, so I think to stay API neutral I should use the erlang calendar library to compute deltas in dates unless you have a different idea? I end up doing something like this for the comparison: :calendar.date_to_gregorian_days({subject.year, subject.month, subject.day}) - : calendar.date_to_gregorian_days({value.year, value.month, value.day})

For point "I." by @bozydar , I find either of the approaches below to be readable...

expect(~N[2017-01-02]).to be :>, ~N[2017-01-01]. I always need to think what does it mean <, > in case of time/date.

expect(~N[2017-01-02]).to be :after, ~N[2017-01-01].

andrei-mihaila commented 7 years ago

Hi,

Do have a look at https://github.com/bitwalker/timex - just for the way it handles comparisons if not to use it as a library (it should make it easy to handle diffs before Elixir 1.5).

I kind of expect expect(~N[2017-01-02]).to be :>, ~N[2017-01-01] to use the > operator, whatever the latter does. So I'd go with expect(~N[2017-01-02]).to be :after, ~N[2017-01-01]. I'm not sure if be_naively is really needed - maybe it can be inferred from the parameters. Also, if comparing dates as naive is desired, some to_naive_date function could be applied before passing the value to expect.

For be_close_to how about (be_close_to ~D[2010-04-17], {:day, 1} )? I think this is needed, especially for the dates with timezones observing daylight saving time (where, for example, one day doesn't always equal 24 hours).

Hope this helps.

treble37 commented 7 years ago

Thanks @andrei-mihaila - what do you all think of this implementation https://github.com/antonmi/espec/pull/226 ? I ported over the Timex comparison logic (without adding Timex as a dependency)....(I was also going to create a Types.ex file for the documenation specs assuming people liked this approach). If you're ok with this approach, I'll update the ExUnit tests for #226 and then do another PR for DateTime perhaps.... cc @antonmi

antonmi commented 7 years ago

Hi @treble37 ! Thanks for awesome work! As I understand we need to implement the protocol for Time and NaiveDateTime. So be_close_to will work for any datetime type.

My suggestions:

Thank you again!

P.S. @andrei-mihaila I look forward to your comments)

andrei-mihaila commented 7 years ago

Hi, @treble37,

It looks good so far (and not depending on an external library is even better)!

I agree with @antonmi's suggestions. Even if it takes longer I think we should complete this feature before merging (so espec should be able to handle all the date types currently in Elixir, including the naive ones and those with time zones).

Thank you!

bozydar commented 7 years ago

Hi @antonmi , @andrei-mihaila and @treble37! It looks like I'm late. Anyway I implemented a plugin as @antonmi suggested although it needs Elixir >= 1.5 (diff/2 and diff/3 are not implemented in Elixr < 1.5). Maybe you find something usable there. https://github.com/bozydar/calendar_espec I implemented: be_timed, be_timed_between and be_timed_close_to.

antonmi commented 7 years ago

Hi, @bozydar ! Awesome work! Thank you!

andrei-mihaila commented 7 years ago

Hi @bozydar,

I find it very useful, nice work!

Thank you!

treble37 commented 7 years ago

Nice @bozydar, the be_timed_close_to, ~D[2017-01-02], [weeks: 1, days: 1] with lists is an interesting api!

Note: The latest preliminary commit https://github.com/antonmi/espec/pull/226/commits/43f2747b854b0713109e74c23b8592e3d3988256 adds NaiveDateTime (still have to update the README, and do a final check of the code, but I think the tests are working correctly) [I hadn't looked at @bozydar work when I made this, but I'll give it a look soon-ish]