Fix getDatesRange so it works regardless of user timezone

[joliss]

Without this fix, I'm getting the following test failures:

  Failed Tests: There were 7 failures

    can get range

      ✖ should be deeply equivalent (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:9:5)

    same start and end

      ✖ should be deeply equivalent (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:17:5)

    works for daylight savings

      ✖ should be deeply equivalent (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:26:5)

    works at month turnover

      ✖ should be deeply equivalent (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:40:5)

    works at year turnover

      ✖ should be deeply equivalent (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:53:5)

    works at leap year

      ✖ leap year (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:66:5)
      ✖ non-leap year (|- not deepEqual |- at: tests/unit/events/regenerator/fire-events/_get-date-range-test.js:74:5)

I grepped the source to see where getDatesRange is used, and it turns out it's used only in one place: https://github.com/covidatlas/li/blob/487c670191800daf93e024ec0a8f39db4779686d/src/events/regenerator/fire-events/index.js#L14

[jzohrab]

Weird, tests are passing for me on master, including these ones.

$ node tests/unit/events/regenerator/fire-events/_get-date-range-test.js
TAP version 13
# can get range
ok 1 should be deeply equivalent
# same start and end
ok 2 should be deeply equivalent
# works for daylight savings
ok 3 should be deeply equivalent
# works at month turnover
ok 4 should be deeply equivalent
# works at year turnover
ok 5 should be deeply equivalent
# works at leap year
ok 6 leap year
ok 7 non-leap year
# throws on non dates
ok 8 should throw
# throws if earlier > later
ok 9 should throw
# throws if missing date
ok 10 should throw

1..10
# tests 10
# pass  10

and when I make the same change:

-    d.setDate(d.getDate() + 1)
     dates.push(datetime.getYYYYMMDD(d))
+    d.setDate(d.getDate() + 1)

most things fail. E.g.,

  actual = getDatesRange('2021-02-27', '2021-03-01')
  expected = [
    '2021-02-27',
    '2021-02-28',
    '2021-03-01'
  ]
  t.deepEqual(expected, actual, 'non-leap year')

with this change, the method now returns [ '2021-02-26', '2021-02-27', '2021-02-28' ], which fails.

All that said, the way you wrote it clearly looks more correct -- we pass it a minimum and max date, and the first thing it should do is push the minimum date, not increment it and then push it. So I can't sort out a) why it works on my machine, and not yours, and b) why the more correct-looking way of writing it is behaving incorrectly on my machine.

So, I don't want to merge this one yet, until we can sort it out!

[jzohrab]

I wonder if this is timezone-related ... I'm running these in Canada, around Toronto time, you'll be in London, closer to GMT ... I can't see why that would be the case, but dates in js are weird.

[joliss]

Ah, yes, this does seem to be a timezone problem. I can reproduce what you're seeing if I run TZ=America/Toronto node tests/unit/events/regenerator/fire-events/_get-date-range-test.js. With the TZ environment variable set, master passes, and my branch fails.

I think the best solution is to use the popular Moment.js library in place of the abominable Date class. I've pushed an update to this pull request to this effect. It now passes with TZ=America/Toronto, TZ=UTC, and TZ=Asia/Tokyo.

Do you want to take another look?

In the future, I think it might make sense to

• run the test suite on CI with multiple timezones, to catch these kinds of issues, and
• progressively phase out usage of Date in favor of moment across the codebase, to make it more robust.

Do you think that plan of action makes sense in general?

[jzohrab]

Hi @joliss -- We're using spacetime already, does that cover what Moment does? Is Moment a better package in general?

[joliss]

I have no opinion or preference between the two. The only reason I went for moment is that it's extremely popular (npm shows 12m weekly downloads, vs. 8k for spacetime), but if we're using spacetime already we might as well use it here.

I pushed this branch again, and I also added a second commit that renames the function to be in line with the filename. I recommend you review the second commit separately because it's just a search-and-replace.