Nondeterministic unit tests

Unit tests are assuming the user has not modified the default culture settings. I have the regional format set as English (United States) on my machine, but have customized some of the default formats under the Date & time menu in the Windows settings app. This is my config:

And with these settings, five unit tests are failing for me. One example below:

Nice find, these might be bugs or unit tests that need to force the culture (in the options, like we do for several ones already).

The culture is specified, it's just that the expected outcome is hardcoded and doesn't match with the computer configuration.

Is this what you mean by forcing the culture - https://github.com/sebastienros/fluid/blob/main/Fluid.Tests/MiscFiltersTests.cs#L259 (this is one of the tests that fails), or am I misunderstanding?

the expected outcome is hardcoded

Hard coded as in "this is exactly what should be rendered, based on the current culture options and any server configuration.

Is this what you mean by forcing the culture

Yes, some culture-dependent tests/templates will have that to ensure that the "hard-coded" expected result is deterministic. I am never setting the current thread culture to a weird one it might blead to other tests that are executed concurrently (there is a risk with async that the code runs on two different threads). Another option would be to run the whole set of tests with a different culture to be certain that we didnt' miss the option in some places, or that the configure culture is correctly taken into account.

Specifying a second parameter useUserOverride: false fixes this issue. Acceptable solution?

new CultureInfo("en-US", useUserOverride: false)

Oh, I see what you mean now, that's a different issue. I am well aware of this option as we are currently discussing its usage in ASP.NET request localization and Orchard Core ;)

For the tests yes we should do that such that the environment doesn't break them.

So the above overload fixed 4/5 tests.

However, there's this one problematic test case left -

fluid/Fluid.Tests/MiscFiltersTests.cs

Line 413 in 63bade9

[InlineData("0", "00:00:00.000")]

When calling FluidValue.Create(timeSpan, options); with timespan all zeroes, the following line

fluid/Fluid/Values/FluidValue.cs

Line 170 in 63bade9

return new DateTimeValue(new DateTime(timeSpan.Ticks));

throws

Message: 
    System.ArgumentOutOfRangeException : The UTC time represented when the offset is applied must be between year 0 and 10,000. (Parameter 'offset')

  Stack Trace: 
    DateTimeOffset.ValidateDate(DateTime dateTime, TimeSpan offset)
    DateTimeOffset.ctor(DateTime dateTime)
    DateTimeOffset.op_Implicit(DateTime dateTime)
    FluidValue.Create(Object value, TemplateOptions options) line 170
    MiscFiltersTests.DateTimeSpan(String timespan, String expected) line 421
    --- End of stack trace from previous location where exception was thrown ---

You have added this recently, any idea why this is failing locally for me?

You have added this recently, any idea why this is failing locally for me?

What culture and TFM are you running on?
Try to change to English or use a different TFM (if one works) and debug to see the differences.

I might handle this because I already worked in exact usecase in Orchard Core

Failing on all - netcoreapp 3.1, net 5, and net 6. The below code throws locally, so it's something for sure on my end

https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQGUCWBbADgGwgAUEB7AcwSm2QBoYQE4A7AHwAEAmARgFgAoAQDcoCAAQATWBBg4IYgLxjmEAO5iAItIAqcgBQAGAJQBuYaLGkwYZDMXK1mnXIDy12zD1SYMuaYFWNjIAdACSzMi4EADGnv78QA==

I can open a PR for the rest for now..

I might handle this because I already worked in exact usecase in Orchard Core

Something to do with my time zone it seems.

// this throws :(
var datetime = new DateTime(0).AddMinutes(59);
var offset = new DateTimeOffset(datetime);

// this works :)
var datetime = new DateTime(0).AddMinutes(60);
var offset = new DateTimeOffset(datetime);

The offset is becoming a negative when new DateTime(0) is passed into it.

What is your timezone?

If that is timezone dependent then we might need to force one in the DateTimeOffset constructor, or the new DateTime(0)

My time zone is set to UTC+1, which explains the 60 minute test.

the new DateTime() assumes "Local" AFAIR, that might be the issue here when passed to new DateTimeOffset.

@danielmpetrov let me knoe if you need help on this, I can push a PR to continue what you already started with

Can't repro the issue:
https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLRLKsBLCAbAE2QBoYQE4A7AHwAEAmARgFgAoNgNygQAIBjOHnhIeAXh6UIAdx61GAOgDieAPbAoebAC9Y2FZXkBhQcIgBJSmBUAKAEQEIt4jzAbkEAJQBuNnPkAVAAskKAJsSgBzAODoAiNEJEoYIJC44wREmGMhRAgxfhNcn1Y/FNjwqLLQ+IyIJKq0hLqYAFUzbNN8gRykYs5uHgIYfMkZABFYCH9sAFsIawAGD3kAQQICAFlwuBgUawBWAE5vft4hlRHpHgnd6bmAeTAwdxhrIZP2EsZDt5gPuR+528QA=

@sebastienros in your code on SharpLab, the offset it printed as +00:00, I get +01:00 locally.

var datetime = new DateTime(0).AddMinutes(60);
var offset = new DateTimeOffset(datetime);
Console.WriteLine(offset); // 01-Jan-01 01:00:00 +01:00

@hishamco If you have an idea go ahead, I have nothing right now. 😁

Maybe SharpLab is highjacking the culture. The challenge is to create some code that will fail with the same error on any machine. I will try the same code on my local machine then.

@hishamco If you have an idea go ahead, I have nothing right now. 😁

Ok, it's in my plate now ..

@sebastienros

From the docs of the constructor - https://learn.microsoft.com/en-us/dotnet/api/system.datetimeoffset.-ctor?view=net-6.0

If the value of DateTime.Kind is DateTimeKind.Utc, the DateTime property of the new instance is set equal to dateTime, and the Offset property is set equal to Zero.

If the value of DateTime.Kind is DateTimeKind.Local or DateTimeKind.Unspecified, the DateTime property of the new instance is set equal to dateTime, and the Offset property is set equal to the offset of the local system's current time zone.

So specifying DateTimeKind.Utc in the constructor would solve this problem. By default, calling new DateTime(0) sets the DateTimeKind property to Unspecified.

var datetime = new DateTime(0, DateTimeKind.Utc); // this clears up the offset
var offset = new DateTimeOffset(datetime); // so this line does not throw anymore
Console.WriteLine(offset); // 01-Jan-01 00:00:00 +00:00

fluid/Fluid/Values/FluidValue.cs

Line 170 in 63bade9

return new DateTimeValue(new DateTime(timeSpan.Ticks));

Tests pass, I don't know if anything else would break conceptually.

Awesome, just submit the PR, with a comment on why the UTC is used in the ctor. Maybe check if we use the same TZ-less constructor somewhere else.

@danielmpetrov seems you did some progress on this, to save the time for us I will stop working on this, and let you start a PR