Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

permissionSelectors tests: Stop using date-fns to mock waiting period #5402

Merged
merged 2 commits into from
Jun 8, 2022

Conversation

chrisbobbe
Copy link
Contributor

Could some unintended behavior in addDays have caused the flake
discussed at
#5401 (comment)
?

In any case, this seems better because we don't have to think about
what special logic addDays might have.

Could some unintended behavior in addDays have caused the flake
discussed at
  zulip#5401 (comment)
?

In any case, this seems better because we don't have to think about
what special logic addDays might have.
@gnprice
Copy link
Member

gnprice commented Jun 8, 2022

Hmm, yeah. I don't quite see how it could have caused that, but good to get rid of it. Looks good -- please merge at will.


For this one, the date-fns jsdoc isn't very helpful:

/**
 * @name addDays
 * @category Day Helpers
 * @summary Add the specified number of days to the given date.
 *
 * @description
 * Add the specified number of days to the given date.

But the implementation is this:

  date.setDate(date.getDate() + amount);

And Date#setDate means:

The setDate() method changes the day of the month of a given Date instance, based on local time.

If the dayValue is outside of the range of date values for the month, setDate() will update the Date object accordingly.

For example, if 0 is provided for dayValue, the date will be set to the last day of the previous month.

That means that e.g. addDays(date, 1) could add 23 hours or 25 hours instead of 24, if the local timezone entered or left DST in between the original time and the new one. More generally, addDays could apply a length of time other than the expected number of multiples of 24 hours, if the local timezone changed its offset in between. In the most extreme cases, I think addDays(date, 2) could add 72 hours, or just 24 hours, if running in a local timezone that shifted from one side of the International Date Line to the other.

@chrisbobbe chrisbobbe merged commit d08df73 into zulip:main Jun 8, 2022
@chrisbobbe
Copy link
Contributor Author

Thanks for the review!

Looks good -- please merge at will.

Done.

@chrisbobbe chrisbobbe deleted the pr-permissions-no-date-fns branch June 8, 2022 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants