HangfireIO / Cronos

A fully-featured .NET library for working with Cron expressions. Built with time zones in mind and intuitively handles daylight saving time transitions
MIT License
1.01k stars 118 forks source link

Possible edge condition problem #53

Closed MelGrubb closed 9 months ago

MelGrubb commented 2 years ago

I could very well be wrong about this. That happens a lot, but given the following setup, I'd expect all four tests to pass here.

private readonly TimeZoneInfo EST = TimeZoneInfo.FindSystemTimeZoneById("America/New_York");
private readonly DateTimeOffset Jan1st2000 = new DateTimeOffset(2000, 01, 01, 00, 00, 00, new TimeSpan(-4, 0, 0));

// Asking just BEFORE midnight should yield the 1st (Works)
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000.AddMilliseconds(-1), EST).ShouldBe(new DateTime(2000, 01, 01));

// Asking exactly AT midnight should also yield the 1st (Works)
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000, EST).ShouldBe(new DateTime(2000, 01, 01));

// Asking just AFTER midnight should yield the 8th (Only the first one works)
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000.AddHours(1), EST).ShouldBe(new DateTime(2000, 01, 08));
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000.AddSeconds(61), EST).ShouldBe(new DateTime(2000, 01, 08));

Once I'm outside the first hour, it correctly jumps to the next Saturday, but shouldn't moving outside the first minute also yield the next Saturday? The individual fields are supposed to be AND-ed together, right? If I've moved my "now" outside the first minute of the first hour, then it shouldn't count again until the next Saturday midnight... right?

Apologies if I'm overlooking the obvious here. I've been staring at this too long.

odinserj commented 2 years ago

Thanks for the detailed description, I will definitely look into this in the nearest future!

odinserj commented 9 months ago

I have finally reviewed the tests, and even added slightly modified version of them to Cronos' unit tests, please see: https://github.com/HangfireIO/Cronos/blob/fe555548cbb1e713ac16824f71b0c5917eb66ffa/tests/Cronos.Tests/CronExpressionFacts.cs#L2434-L2454.

In short, everything works as expected here, let me comment line by line.

private readonly DateTimeOffset Jan1st2000 = new DateTimeOffset(2000, 01, 01, 00, 00, 00, new TimeSpan(-4, 0, 0));

Since we are using EST, I have modified UTC Offset to its correct one, UTC +05:00 here, according to this article and .NET's BaseUtcOffset (unchanged since that time).

// Asking exactly AT midnight should also yield the 1st (Works)
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000, EST).ShouldBe(new DateTime(2000, 01, 01));

This test shouldn't be working, since by default GetNextOccurrence doesn't include the current time (inclusive parameter set to false), and should emit next SAT instead.

// Asking just AFTER midnight should yield the 8th (Only the first one works)
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000.AddHours(1), EST).ShouldBe(new DateTime(2000, 01, 08));
CronExpression.Parse("0 0 * * 6").GetNextOccurrence(Jan1st2000.AddSeconds(61), EST).ShouldBe(new DateTime(2000, 01, 08));

These tests are working just fine.

Please try to run the updated test suite that include your tests now and tell me exactly error messages if you do have any.

odinserj commented 9 months ago

I'm closing this now, please feel free to re-open it if you have any troubles with tests!