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

Update the Date data set to avoid DST transition windows #366

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

logiclrd
Copy link
Contributor

This PR:

  • Adds a unit test that I'm pretty sure will find a DST window and force a value to generate inside of it. Works on my system, anyway. The test should automatically skip if the local time zone doesn't do DST transitions.
  • Updates the implementation of the Date data set so that all requests get funnelled into Between and BetweenOffset, and then reimplements Between and BetweenOffset to convert the range to UTC first, then obtain a random value, and then convert it back to a local time if appropriate before returning it, taking advantage of the framework's daylight savings transition calculations.

The unit test fails with the original Date data set code, and the change in the Date data set fixes the unit test. On my system, at least.

This PR is in response to #365.

@logiclrd
Copy link
Contributor Author

Rebased. I thought I was working with the latest master, but I wasn't. The change is now in terms of the latest master.

@logiclrd logiclrd force-pushed the JDG_DateTimeAvoidDSTTransition branch from 3489f09 to b3f99bf Compare March 16, 2021 04:46
@bchavez bchavez self-requested a review March 21, 2021 15:35
@bchavez bchavez self-assigned this Mar 21, 2021
@bchavez bchavez linked an issue Mar 21, 2021 that may be closed by this pull request
Comment on lines 166 to 168
var dateTime = new DateTime(minTicks, DateTimeKind.Unspecified) + partTimeSpan;

return new DateTimeOffset(dateTime + start.Offset, start.Offset);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to replace these two lines with:

         if( start < end )
            return start + partTimeSpan;
         else
            return end + partTimeSpan;

Given an invalid start time; the function will still return an invalid DateTimeOffset.

image

With the code change, at least the method returns a valid time, albeit outside the "specified" time range; which is fine I guess, because as it stands now, it would still have been outside the "2:59 AM - 3:00 AM" time as noted above.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems legit :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess to my mind, if the caller supplies impossible DateTime values to begin with, all bets are off. My specific use case doesn't intersect with that, so I don't have strong opinions about how exactly it behaves in that edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking it through further, if one wanted to be really thorough, then the algorithm would need to include some sort of "compute real DateTime range" functionality, so that start and end were adjusted if they lay inside an impossible time span, and this would then also throw if start and end both lay within the same transition window (so that no real date/time x would satisfy start <= x <= end). This would be in addition to generating the actual bogus value in UTC, to avoid transition windows in the middle of the range. I think this would be the most polished form of this, but I'm not sure if it's worth the effort.

Copy link
Contributor Author

@logiclrd logiclrd Mar 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pseudocode:

void ComputeRealRange(ref DateTime start, ref DateTime end)
{
  if (start > end)
    Swap(ref start, ref end);

  var (windowStart, windowEnd) = GetForwardDSTTransitionWindow(start.Year);

  if (windowStart <= start <= windowEnd)
    start = windowEnd;

  (windowStart, windowEnd) = GetForwardDSTTransitionWindow(end.Year);

  if (windowStart <= end <= windowEnd)
    end = windowStart;

  if (start > end)
    throw new Exception("Impossible");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach, a supplied range as shown where start lay within the impossible range and end was exactly the first next valid DateTime value, it would always return exactly the end value because the effective start would be bumped to the end of the transition window and would coincide with end.

@logiclrd
Copy link
Contributor Author

Any changes you'd like to request on this PR? :-)

@logiclrd
Copy link
Contributor Author

logiclrd commented Apr 1, 2021

I have implemented the proposed algorithm for excluding impossible values from the range by bumping start forward and end back, along with unit tests that detect the correction.

@logiclrd logiclrd force-pushed the JDG_DateTimeAvoidDSTTransition branch 2 times, most recently from 774684d to 4660f9c Compare April 1, 2021 17:38
@logiclrd
Copy link
Contributor Author

logiclrd commented Apr 1, 2021

There were some shenanigans surrounding Kind and offsets but that's now sorted out. For your test, with start set to 1 minute before the end of the transition and end set to exactly the end of the transition, the requested range actually only has one real DateTime value, that being the exact end of the transition, and so it will always return that exact value.

2021-03-14 2:59:00 AM -06:00
2021-03-14 3:00:00 AM -05:00
2021-03-14 8:59:00 AM +00:00
2021-03-14 8:00:00 AM +00:00
2021-03-14 3:00:00 AM -06:00

@logiclrd
Copy link
Contributor Author

logiclrd commented Apr 1, 2021

I have found an edge case :-) If the requested range starts exactly at the beginning of a transition window and ends exactly at the end of a transition window, then it fails to adjust. Compensating for that.

@logiclrd logiclrd force-pushed the JDG_DateTimeAvoidDSTTransition branch from 50b2b1f to 347efeb Compare April 1, 2021 18:02
@logiclrd
Copy link
Contributor Author

logiclrd commented Apr 1, 2021

Sorry, I don't normally do so many force-pushes, but in my eagerness I'm afraid I pushed changes only to realize a few minutes later that I had overlooked a case and return to the code to address it.

@bchavez
Copy link
Owner

bchavez commented Apr 1, 2021

@logiclrd no worries. Thank you for making the improvements; sorry for the delay getting this merged. I've been busy with a few life events that are taking a bit of time to resolve.

{
public FactWhenDaylightSavingsSupported()
{
if (!TimeZoneInfo.Local.SupportsDaylightSavingTime)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I had no clue this was a thing.

Comment on lines +372 to +388
var faker = new Faker();

faker.Random = new Randomizer(localSeed: 5);

var dstRules = TimeZoneInfo.Local.GetAdjustmentRules();

var now = DateTime.Now;

var effectiveRule = dstRules.Single(rule => (rule.DateStart <= now) && (rule.DateEnd >= now));

var transitionStartTime = CalculateTransitionDateTime(now, effectiveRule.DaylightTransitionStart);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New to this code base, but what's the consensus on generating test helper methods or classes that can contain boiler plate construction and arrangement of objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My convention when writing tests has been to make the test methods completely logically independent of one another, but I have seen tests and test suites that reuse a great deal of setup. This could be refactored, but was written this way intentionally (if without a great deal of thought, per se).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer consolidation of the test initialization, what's your opinion @bchavez?

Comment on lines +401 to +420
var faker = new Faker();

faker.Random = new Randomizer(localSeed: 5);

var dstRules = TimeZoneInfo.Local.GetAdjustmentRules();

var now = DateTime.Now;

var effectiveRule = dstRules.Single(rule => (rule.DateStart <= now) && (rule.DateEnd >= now));

var transitionStartTime = CalculateTransitionDateTime(now, effectiveRule.DaylightTransitionStart);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we can probably toss all of this initial setup into a base method (except for the rule) and just call the method at the beginning of the tests

@logiclrd
Copy link
Contributor Author

@bchavez Ping :-)

@bchavez
Copy link
Owner

bchavez commented Sep 14, 2021

Hey @logiclrd , im still here :) I'll try to give it a shot this weekend!!

@logiclrd
Copy link
Contributor Author

@bchavez How's it going? :-D

@bchavez
Copy link
Owner

bchavez commented Jan 26, 2022

hey @logiclrd , for sure, I need to get back to this - I just haven't found enough time to do the extensive testing I want to do before I merge this PR

@logiclrd
Copy link
Contributor Author

If you can identify what the actual tests are, I could look at creating automated tests to cover that as part of this PR.

@bchavez
Copy link
Owner

bchavez commented Feb 3, 2022

@logiclrd , it was the same test I did that found the previous issue in the PR here:

I know it is probably fixed, but I need to see it first-hand and probably see how it behaves in other scenarios too.

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 3, 2022

The code has changed since then. I'll run the same test, see what happens. :-)

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 3, 2022

Okay, I added a test that I think encapsulates what you were talking about, and I tweaked the behaviour of the Date set to match the expectations encoded in the tests.

@logiclrd
Copy link
Contributor Author

Let me know if that test covers the functionality you were thinking of. :-)

@bchavez
Copy link
Owner

bchavez commented Feb 20, 2022

Thank you @logiclrd -- I appreciate it; hopefully, after I get personal and business taxes out of the way I can take a 2nd look. Just really difficult to do OSS work after a full-time job; and I get pretty tired by the end of the day.

There's always a risk; because as soon as I merge and release this code, I have to be ready for an influx of support requests which is why I'm waiting until I have enough buffer-time to handle any immediate issue that could come up as a result of some new time code bug that could come up as a result of this new code.

Yes, we do have a bug now, but at least it's known -- and hasn't caused any major problems as far as GH issues are concerned.

@logiclrd
Copy link
Contributor Author

That's fair :-) For what it's worth, we've been running on a private prerelease build on our private Azure Artifacts server for, well, for some time now :-P

@bchavez bchavez force-pushed the master branch 29 times, most recently from 0b65acc to cc799df Compare December 22, 2023 19:04
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.

Bogus can generate impossible DateTime values
5 participants