Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@pengowray
Copy link

@pengowray pengowray commented Dec 8, 2018

These two tests are expecting buggy behavior and should be removed.

When TimeSpan.Parse() is given one more fractional digit than is expected, it sometimes multiplies the result by 10. This shouldn't be expected behavior. I've detailed this bug here:

You can see there's something off just eyeballing the source code and how the pattern breaks in the last line:

yield return new object[] { "1:1:.01", CultureInfo.InvariantCulture, new TimeSpan(36600100000) };
yield return new object[] { "1:1:.001", CultureInfo.InvariantCulture, new TimeSpan(36600010000) };
yield return new object[] { "1:1:.0001", CultureInfo.InvariantCulture, new TimeSpan(36600001000) };
yield return new object[] { "1:1:.00001", CultureInfo.InvariantCulture, new TimeSpan(36600000100) };
yield return new object[] { "1:1:.000001", CultureInfo.InvariantCulture, new TimeSpan(36600000010) };
yield return new object[] { "1:1:.0000001", CultureInfo.InvariantCulture, new TimeSpan(36600000001) };
yield return new object[] { "1:1:.00000001", CultureInfo.InvariantCulture, new TimeSpan(36600000001) };

Perhaps, it might, in some bizarre way, seem reasonable for "0.1 ticks" to round up to 1 tick (that's what the final test is effectively checking for). But with a little investigation, it's clearly a bug. If you replace 1 with 2 could also be checking that 0.2 ticks "rounds up" to 2 ticks, and this would "pass" also:

yield return new object[] { "1:1:.0000002", CultureInfo.InvariantCulture, new TimeSpan(36600000002) };
yield return new object[] { "1:1:.00000002", CultureInfo.InvariantCulture, new TimeSpan(36600000002) };

This is not how numbers work.

Or you could check that 100000 ticks (0.01s) == 1000000 ticks (0.1s), and you'd find this to also pass. Here you can see this behavior in action

Eventually the test should be replaced with something checking for sane behavior. It should really be rounding the fractional digits in the same way that decimal.Parse() works. Failing that, it should at least throw a "Format" or even an "Overflow" exception (though extra decimal places isn't really an overflow). However it's fixed, these two tests are checking for incorrect behavior and should be removed.

There's more discussion of the bug, a patch for the immediate problem, and discussion of better possible ways to fix it at dotnet/coreclr#21077

Don't expect seconds with 8 fractional digits to be multiplied by 10x
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need test(s) that verify the fix going into CoreCLR presumably. Why not roll those into this PR, and we can merge it when we pick up the CoreCLR fix?

@karelz
Copy link
Member

karelz commented Dec 19, 2018

Agreed with @danmosemsft ... I thought that what PR #33581 was for ... to add/change tests for the new CoreCLR change.

@tarekgh
Copy link
Member

tarekgh commented Jan 12, 2019

Closing this one and I'll open a new one to match my new PR dotnet/coreclr#21968

@tarekgh tarekgh closed this Jan 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants