Update the Date data set to avoid DST transition windows

[logiclrd]

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]

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]

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

[logiclrd]

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]

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]

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]

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]

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

[logiclrd]

@bchavez Ping :-)

[bchavez]

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

[logiclrd]

@bchavez How's it going? :-D

[bchavez]

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]

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]

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

• https://github.com/bchavez/Bogus/pull/366#discussion_r598342181

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]

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

[logiclrd]

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]

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

[bchavez]

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]

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]

How's it going? :-)

[logiclrd]

Where are we at?

[voronoipotato]

Just hit this trying to test an issue I was having where the unit test was working locally but failing on the build server configured to use UTC.

      Expected: ···2022-03-29 05:46:04Z
      Actual:   ···2022-03-29 05:46:03Z 

building this test was actually quite difficult, because the faker facade really really doesn't want to let go of the seed even though I created a whole new instance. Turns out you have to set the seed on the original faker instance and not the new one, but you also do need to create a new instance, and you need to do it before you update the seed.

System.Threading.Thread.CurrentThread.CurrentCulture <- CultureInfo.InvariantCulture
let faker =Faker(Random = (Randomizer(777), Localization = "en")
let data = generateDates faker //some code to make a ton of dates
let faker2 =Faker(Random = (Randomizer(777), Localization = "en")
faker.Random <- Randomizer(777) 
//if I don't do this for some reason faker produces entirely different datasets even though faker2 should
// be an entirely different instance, also setting faker.Random on faker 2 does nothing at all. 
let data2 = generateDates faker2
Assert.IsTrue(data.ToString() = data2.ToString())

probably going to create a new issue for this since it can make it nearly impossible to replicate bugs without determinism, and it would be nice if separate faker instances were at least "able" to be completely separate.

[logiclrd]

@bchavez Any chance of getting this loose end tied off? :-)

[logiclrd]

Oh, you know what, I was mistaken. We don't have a custom build of Bogus. We have a custom build of FluentAssertions with support for System.Data types. We just still get random test failures around DST transition. Another developer at some point made it retry tests up to 5 times to handle stochastic failures. That's its own thing :-P

[logiclrd]

Maybe we can get this in place before the next DST switch? :-)

[logiclrd]

The next DST switch is in 5 days.

[logiclrd]

Well, I guess the next DST switch that matters is March 12, 2023. :-P

[logiclrd]

@bchavez Ping :-)

[logiclrd]

10 days away!

[logiclrd]

Hello from the other side :-)

[bchavez]

Hey @ChristoWolf, if you have a chance or any free time;

Could you double-check this PR and see if we have sufficient unit test coverage for edge cases? I'd like to know your thoughts on this PR.

I'm curious if there are any edge cases where this new code might break existing code. Or ways we could intentionally break the new code or its assumptions.

Right now, currently working on fixing the CI/CD build system and getting the pipeline up and running again. After that I'll likely switch gears to this PR; but would be nice to have a fresh set of eyes and opinions on it.