DateTime util module and tests #2372
Conversation
|
|
||
| alias Dotcom.Utils | ||
|
|
||
| @type service_range() :: :past | :today | :this_week | :next_week | :later |
There was a problem hiding this comment.
(Non-blocking) This name (and the name of the function below that's also called service_range/1) feels like a potential source of confusion, since it refers to an atom, not an actual range.
|
|
||
| The service range continuum: | ||
|
|
||
| <---before today---|---later this week---|---next week---|---after next week---> |
There was a problem hiding this comment.
Ooh this is nice. Love a good explanatory ASCII art.
lib/dotcom/utils/date_time.ex
Outdated
| That is because one hour does not occur--02:00:00am to 03:00:00am. | ||
| In that case, we set the time to 03:00:00am. | ||
|
|
||
| If we are given something tha tis not a DateTime, AmbiguousDateTime, or an error tuple, we log the input and return `now`. |
There was a problem hiding this comment.
(Non-blocking): It looks like the now fallback was removed (thank you 🙂), so unless I missed something, I think we can also drop this part of the doc comment?
There was a problem hiding this comment.
Also a small nit, but if I did miss something then here's a tiny spelling fix.
| If we are given something tha tis not a DateTime, AmbiguousDateTime, or an error tuple, we log the input and return `now`. | |
| If we are given something that is not a DateTime, AmbiguousDateTime, or an error tuple, we log the input and return `now`. |
There was a problem hiding this comment.
Left a few more comments, but you'll have to address them as follow-ups if you feel they're worth addressing at all. Ha. Ha!
(Maybe I'll address them myself if I think they're worth addressing - anything can happen!)
But seriously, this is great - big improvement over the mishmash of Utils.service_* and Timex calls, and nice to have a unified place for core service and core date-time stuff.
| The service range for the given date_time. | ||
| """ | ||
| @spec service_range(DateTime.t()) :: named_service_range() | ||
| def service_range(date_time) do |
There was a problem hiding this comment.
(Non-blocking) Having the word _range at the end of the function name service_range makes me think that it returns something with the type date_time_range(), much like the other service_range_day()/etc functions. (Same deal with the name of the type named_service_range() - ending it with _range() makes me think that
It's not a huge deal, especially because dialyzer will help keep callers of this function honest, but still a potential source of confusion.
Some options:
service_window()ornamed_service_window()for both the function and the type?- Augment the
@dochere to include examples.
This is kind of what I was getting at with this comment
| A behaviour for working with date_times. | ||
| """ | ||
|
|
||
| @callback now() :: DateTime.t() |
There was a problem hiding this comment.
Question (non-blocking): now() is the only one of these functions that we would ever need to mock, right?
Does that mean that the other two functions could be regular functions, rather than callbacks?
(Disclaimer: I'm new to Elixir behaviours ((to the point of never being sure how to even spell it)), so there's a lot I don't know about how they work and what they're for)
|
|
||
| test "returns :today if today is the last day of the service week" do | ||
| # Setup | ||
| expect(Dotcom.Utils.DateTime.Mock, :now, 2, fn -> |
There was a problem hiding this comment.
Suggestion: Instead of mocking now twice in this test, pass Dotcom.Utils.DateTime.now() |> Timex.beginning_of_week() |> beginning_of_service_day() into service_range_later_this_week() as an argument.
I think that in order for that to work fully, we'd need to replace the expect calls with stub in this test.
| # Setup | ||
| {beginning_of_service_week, end_of_service_week} = service_range_later_this_week() | ||
|
|
||
| stub(Dotcom.Utils.DateTime.Mock, :now, fn -> |
There was a problem hiding this comment.
Curiosity: How come we use stub sometimes in this suite, and sometimes we use expect?
This is just some background work to system status and planned disruptions. Most of the logic entails checking the time range of an alert against various service ranges. This elevates the concept of a service range in a similar way that
Timexhas functions likebeginning_of_weekorend_of_week. But, instead we havebeginning_of_service_weekandend_of_service_week.The tests introduce a new property-based testing framework which is great for catching edge cases like daylight savings time.