SP-2356: Fixed flaky DateTimeExtensionTests

[tombogle]

The tests that were previously commented out because of their dependency on certain current dates should now be reliable.

---

This change is 

[github-actions]

Palaso Tests

     4 files  ± 0       4 suites  ±0   9m 40s ⏱️ +6s
 5 055 tests +10   4 821 ✅ +10  234 💤 ±0  0 ❌ ±0 
16 465 runs  +30  15 744 ✅ +30  721 💤 ±0  0 ❌ ±0 

Results for commit 4de7a03. ± Comparison against base commit 8c1f89c.

♻️ This comment has been updated with latest results.

[imnasnainaec]

@imnasnainaec reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @andrew-polk)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

		[TestCase("d/M/yyyy")] // Thai/European-style numeric date (e.g. 14/5/2025)
		[TestCase("d-M-yyyy")] // Thai/European-style numeric date with dashes (e.g. 14-5-2025)
		[TestCase("d")] // Short date pattern (e.g. 14/5/2025)

The fact that the "d" format was failing confirms that (at least with a US region), it's (e.g. 5/14/2025) and not (e.g. 14/5/2025).

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

		[TestCase("d/M/yyyy")] // Thai/European-style numeric date (e.g. 14/5/2025)
		[TestCase("d-M-yyyy")] // Thai/European-style numeric date with dashes (e.g. 14-5-2025)
		[TestCase("d")] // Short date pattern (e.g. 14/5/2025)

The two "d" cases are failing for me locally.

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 428 at r1 (raw file):

		public void ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear(string inputFormat)
		{
			var futureDate = GetNextFutureDateWithPotentiallyAmbiguousDay();

The issue I have with this workaround is that it hides the fact that this function isn't actually handling all the date formats used in the test-cases. Somebody using the US-style numeric date may have future dates in the current year that will be parsed as past dates, causing the function to act differently than expected. Perhaps adding a remark to the function about what formats are expected would be better than writing tests that hide a gap in coverage.

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 565 at r1 (raw file):

			var date = DateTime.Today.AddDays(minDaysIntoFuture);
			while (date.Day > 12)
				date = date.AddDays(1);

Or

			if (date.Day > 12)
				date = date.AddMonths(1).AddDays(1 - date.Day);

[tombogle]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @andrew-polk and @imnasnainaec)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

The fact that the "d" format was failing confirms that (at least with a US region), it's (e.g. 5/14/2025) and not (e.g. 14/5/2025).

The "d" (short date) format specifier in .NET is indeed culture-sensitive. (I am changing the comment to better reflect that.) It pulls the pattern from the current culture’s DateTimeFormat.ShortDatePattern.

• In en-US, "d" → M/d/yyyy → 5/14/2025.
• In en-GB or many European locales, "d" → dd/MM/yyyy → 14/05/2025.
• In th-TH (Thai), "d" → d/M/yyyy → 14/5/2025.

Since we already have explicit test cases for each of these, it could probably be argued that there is not a need for a "d" test case at all, but I am wanting to try to have test cases that reflect the sad reality of potentially ambiguous "data in the wild".

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

The two "d" cases are failing for me locally.

I assume you mean that it is still failing locally even with my proposed fix. If you break after the line that assigns input, what does that variable contain? Unless your current culture is set in such a way that it does not resolve to one of the explicit patterns already reflected in the other test cases, I don't see how "d" could fail without the corresponding explicit date pattern case also failing. I have now tweaked the verification logic to account for unexpected input formats due to unusual short date formats, but if its failing for you with a standard US short date, we need to figure out why/how.

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 428 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

The issue I have with this workaround is that it hides the fact that this function isn't actually handling all the date formats used in the test-cases. Somebody using the US-style numeric date may have future dates in the current year that will be parsed as past dates, causing the function to act differently than expected. Perhaps adding a remark to the function about what formats are expected would be better than writing tests that hide a gap in coverage.

I do want the unit tests to be reliable (obviously), but I also want them to stress the method as much as possible, especially since it's apparent that there are some tricky edge cases. The problem is that the actual method being tested is using some somewhat fuzzy rules to make a best guess. I'm not sure what you mean by "this function isn't actually handling all the date formats used in the test-cases". It "handles" (to the best of its ability) anything you throw at it.

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 565 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Or

			if (date.Day > 12)
				date = date.AddMonths(1).AddDays(1 - date.Day);

Good point. I'll do that but in a slightly more readable way.

[imnasnainaec]

@imnasnainaec reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @andrew-polk and @tombogle)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I assume you mean that it is still failing locally even with my proposed fix. If you break after the line that assigns input, what does that variable contain? Unless your current culture is set in such a way that it does not resolve to one of the explicit patterns already reflected in the other test cases, I don't see how "d" could fail without the corresponding explicit date pattern case also failing. I have now tweaked the verification logic to account for unexpected input formats due to unusual short date formats, but if its failing for you with a standard US short date, we need to figure out why/how.

I'll try it again when I get to my Windows work computer. It's passing today on my Mac laptop.

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 428 at r1 (raw file):
That phrase was only referring back to my example that

Somebody using the US-style numeric date may have future dates in the current year that will be parsed as past dates,

[imnasnainaec]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @andrew-polk and @tombogle)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

I'll try it again when I get to my Windows work computer. It's passing today on my Mac laptop.

'Twas

[Failed] ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d")
    Message:
          Should have returned a past year
          Expected: 1482
          But was:  2025
    Stack Trace:
           at SIL.Tests.Extensions.DateTimeExtensionTests.ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear(String inputFormat) in c:\Users\danie\sillsdev\libpalaso\SIL.Core.Tests\Extensions\DateTimeExtensionTests.cs:line 459

Now 'tis

[Skipped] ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d")
    Message:
        "d" test case (dependent on current culture) year 2025 != expected 1482. Input = {input}
    Standard Output Messages:
        input = 2025-09-10        "d" test case (dependent on current culture) year 2025 != expected 1482. Input = {input}

...because I personalized my locale settings with year-first.

[tombogle]

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 428 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

That phrase was only referring back to my example that

Somebody using the US-style numeric date may have future dates in the current year that will be parsed as past dates,

The parsing methods now have the ability for the caller to determine an appropriate (future) cut-off date. In the case of SayMore (which I think is the only app that currently uses this), I think it's not really expected that people would be using it to document future events, so I think I'm okay with the very slight risk of someone having done that (using a really old version of SayMore) and then having the date misinterpreted.

[tombogle]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @andrew-polk and @imnasnainaec)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 421 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

'Twas

[Failed] ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d")
    Message:
          Should have returned a past year
          Expected: 1482
          But was:  2025
    Stack Trace:
           at SIL.Tests.Extensions.DateTimeExtensionTests.ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear(String inputFormat) in c:\Users\danie\sillsdev\libpalaso\SIL.Core.Tests\Extensions\DateTimeExtensionTests.cs:line 459

Now 'tis

[Skipped] ParsePastDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d")
    Message:
        "d" test case (dependent on current culture) year 2025 != expected 1482. Input = {input}
    Standard Output Messages:
        input = 2025-09-10        "d" test case (dependent on current culture) year 2025 != expected 1482. Input = {input}

...because I personalized my locale settings with year-first.

Yes, with a personalized short-date format, I think all bets are off. But I think marking it as ignored/skipped (rather than an actual failure) is a decent compromise.

[imnasnainaec]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 450 at r3 (raw file):

			var futureDate = GetNextFutureDateWithPotentiallyAmbiguousDay();
			var input = futureDate.ToString(inputFormat);
			Console.WriteLine($@"{nameof(input)} = {input}");

Do you want the two Console.WriteLine lines to persist or was that for debugging?

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 479 at r3 (raw file):

			thread.Join();

			if (inputFormat == "d")

I'm not sure this special handling is worthwhile. Do we need to keep the "d" case if it can never cause failure?

[tombogle]

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 479 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

I'm not sure this special handling is worthwhile. Do we need to keep the "d" case if it can never cause failure?

I debated removing it, but decided it was probably still nice to have. It would get reported as a new ignored test if it started to "fail" in the CI build, so there's some chance it could get flagged for further investigation. And it's kind of nice to have around for local builds as well. (I thought about adding special logic to have it fail on local builds and ignore only in a CI build, but that seemed like overkill, and probably a local build is more likely to experience a spurious failure. It's usefulness is definitely borderline, but that's how I see it.

[tombogle]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @andrew-polk and @imnasnainaec)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 450 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Do you want the two Console.WriteLine lines to persist or was that for debugging?

I think so. If there ever is a failure or an ignored case that we want to investigate, having that info in the log would be beneficial.

[imnasnainaec]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 510 at r3 (raw file):

		{
			var futureDate = GetNextFutureDateWithPotentiallyAmbiguousDay(5);
			int maxDateOffset = (futureDate - DateTime.Today).Days - 1;

Is there benefit in calculating maxDateOffset to be one day before the future date? Or can we simply have

			var maxDateOffset = 4;
			var futureDate = GetNextFutureDateWithPotentiallyAmbiguousDay(maxDateOffset + 1);

(Though I think it's more readible to have reasonableMin, reasonableMax, and futureDate all defined together at the start, I can understand conceptually defining reasonableM* with the test culture. )

[tombogle]

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 510 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Is there benefit in calculating maxDateOffset to be one day before the future date? Or can we simply have

			var maxDateOffset = 4;
			var futureDate = GetNextFutureDateWithPotentiallyAmbiguousDay(maxDateOffset + 1);

(Though I think it's more readible to have reasonableMin, reasonableMax, and futureDate all defined together at the start, I can understand conceptually defining reasonableM* with the test culture. )

I think either would be okay. I debated hard-coding maxDateOffset like that. For tightest proof that the code is "correct", using the more precise calculation of maxDateOffset is better, but if it's less clear then maybe it's worse.

[imnasnainaec]

@imnasnainaec reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrew-polk)

---

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 510 at r3 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I think either would be okay. I debated hard-coding maxDateOffset like that. For tightest prof that the code is "correct", using the more precise calculation of maxDateOffset is better, but if it's less clear then maybe it's worse.

I'll approve and leave it to your final judgement.