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

logiclrd · 2021-03-16T01:36:08Z

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 · 2021-03-16T04:41:25Z

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

bchavez · 2021-03-21T21:40:47Z

Source/Bogus/DataSets/Date.cs

+         var dateTime = new DateTime(minTicks, DateTimeKind.Unspecified) + partTimeSpan;
+
+         return new DateTimeOffset(dateTime + start.Offset, start.Offset);


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.

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.

That seems legit :-)

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.

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.

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"); }

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 · 2021-03-27T17:10:53Z

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

logiclrd · 2021-04-01T17:29:19Z

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 · 2021-04-01T17:40:12Z

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 · 2021-04-01T17:43:37Z

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 · 2021-04-01T18:03:11Z

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 · 2021-04-01T22:27:45Z

@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.

Chobocharles · 2021-07-01T03:37:53Z

Source/Bogus.Tests/DataSetTests/DateTest.cs

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


Wow, I had no clue this was a thing.

Chobocharles · 2021-07-01T03:41:09Z

Source/Bogus.Tests/DataSetTests/DateTest.cs

+         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);


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?

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).

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

Chobocharles · 2021-07-01T03:43:24Z

Source/Bogus.Tests/DataSetTests/DateTest.cs

+         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);


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 · 2021-09-13T16:17:17Z

@bchavez Ping :-)

bchavez · 2021-09-14T03:03:47Z

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

logiclrd · 2022-01-24T19:48:30Z

@bchavez How's it going? :-D

bchavez · 2022-01-26T00:46:02Z

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 · 2022-01-31T16:17:46Z

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 · 2022-02-03T03:15:21Z

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

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

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 · 2022-02-03T15:29:17Z

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

logiclrd · 2022-02-03T17:23:22Z

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 · 2022-02-17T08:24:28Z

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

bchavez · 2022-02-20T00:07:51Z

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 · 2022-02-20T01:53:34Z

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

logiclrd mentioned this pull request Mar 16, 2021

Bogus can generate impossible DateTime values #365

Open

logiclrd force-pushed the JDG_DateTimeAvoidDSTTransition branch from 459d369 to 3489f09 Compare March 16, 2021 04:40

logiclrd force-pushed the JDG_DateTimeAvoidDSTTransition branch from 3489f09 to b3f99bf Compare March 16, 2021 04:46

bchavez self-requested a review March 21, 2021 15:35

bchavez self-assigned this Mar 21, 2021

bchavez linked an issue Mar 21, 2021 that may be closed by this pull request

Bogus can generate impossible DateTime values #365

Open

bchavez reviewed Mar 21, 2021

View reviewed changes

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

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

Chobocharles reviewed Jul 1, 2021

View reviewed changes

bchavez force-pushed the master branch 29 times, most recently from 0b65acc to cc799df Compare December 22, 2023 19:04

bchavez force-pushed the master branch from dc27f91 to 85071b2 Compare March 2, 2024 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

logiclrd commented Mar 16, 2021

logiclrd commented Mar 16, 2021

bchavez Mar 21, 2021

logiclrd Mar 21, 2021

logiclrd Mar 21, 2021

logiclrd Mar 21, 2021

logiclrd Mar 21, 2021 •

edited

Loading

logiclrd Mar 21, 2021

logiclrd commented Mar 27, 2021

logiclrd commented Apr 1, 2021

logiclrd commented Apr 1, 2021

logiclrd commented Apr 1, 2021

logiclrd commented Apr 1, 2021

bchavez commented Apr 1, 2021

Chobocharles Jul 1, 2021

Chobocharles Jul 1, 2021

logiclrd Jul 4, 2021

ChristoWolf Mar 18, 2023

Chobocharles Jul 1, 2021

logiclrd commented Sep 13, 2021

bchavez commented Sep 14, 2021

logiclrd commented Jan 24, 2022

bchavez commented Jan 26, 2022

logiclrd commented Jan 31, 2022

bchavez commented Feb 3, 2022

logiclrd commented Feb 3, 2022

logiclrd commented Feb 3, 2022

logiclrd commented Feb 17, 2022

bchavez commented Feb 20, 2022

logiclrd commented Feb 20, 2022

		var dateTime = new DateTime(minTicks, DateTimeKind.Unspecified) + partTimeSpan;

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

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

Are you sure you want to change the base?

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

Conversation

logiclrd commented Mar 16, 2021

logiclrd commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd commented Mar 27, 2021

logiclrd commented Apr 1, 2021

logiclrd commented Apr 1, 2021

logiclrd commented Apr 1, 2021

logiclrd commented Apr 1, 2021

bchavez commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd commented Sep 13, 2021

bchavez commented Sep 14, 2021

logiclrd commented Jan 24, 2022

bchavez commented Jan 26, 2022

logiclrd commented Jan 31, 2022

bchavez commented Feb 3, 2022

logiclrd commented Feb 3, 2022

logiclrd commented Feb 3, 2022

logiclrd commented Feb 17, 2022

bchavez commented Feb 20, 2022

logiclrd commented Feb 20, 2022

logiclrd Mar 21, 2021 •

edited

Loading