Fix a failing BigInteger outer loop test

[Maxwe11]

Ref #1491

[stephentoub]

Can you verify this passes on Linux/OSX as well? It may not, due to #846, in which case we should change the attribute to [ActiveIssue(846, PlatformID.AnyUnix)]. Of course, if it does pass, great. cc: @ellismg

[Maxwe11]

Can you verify this passes on Linux/OSX as well?

I have no available Linux/OSX for now.

❗ With these fixes I'm getting test failure locally

System.Numerics.Tests.parseTest.RunParseToStringTests [FAIL]
    Assert.Throws() Failure
    Expected: typeof(System.FormatException)
    Actual:   (No exception was thrown)
    Stack Trace:
       m:\Workspace\github\corefx\src\System.Runtime.Numerics\tests\BigInteger\parse.cs(408,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected, String expected)
       m:\Workspace\github\corefx\src\System.Runtime.Numerics\tests\BigInteger\parse.cs(380,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected)
       m:\Workspace\github\corefx\src\System.Runtime.Numerics\tests\BigInteger\parse.cs(247,0): at System.Numerics.Tests.parseTest.VerifyNumberStyles(NumberStyles ns, Random random)
       m:\Workspace\github\corefx\src\System.Runtime.Numerics\tests\BigInteger\parse.cs(33,0): at System.Numerics.Tests.parseTest.RunParseToStringTests()
Finished:    System.Runtime.Numerics.Tests
=== TEST EXECUTION SUMMARY ===
System.Runtime.Numerics.Tests  Total: 2, Errors: 0, Failed: 1, Skipped: 0, Time: 20,751s

I think it's a bug. So before merging this, test it locally please with repro steps in #1491

[krwq]

@Maxwe11, I'm not sure I understand your above comment. Does it mean there is a product bug in addition to the test bug or that this fix doesn't fix a test?

[Maxwe11]

Does it mean there is a product bug in addition to the test bug or that this fix doesn't fix a test?

Seems to me yes. I've checked same input with full .NET and it throws FormatException as expected.

[stephentoub]

@Maxwe11, can you share the input you used that's failing on .NET 4.5/6 bug succeeding on .NET Core?

[Maxwe11]

@stephentoub
BigInteger.Parse("1987442956125330092956022233482810406471227749906590072586420956431482462072838" + "\u0020\u0020\u0020", NumberStyles.AllowThousands & NumberStyles.AllowTrailingWhite);
this line throws FormatException under .NET 4.5, but under .NET Core it doesn't throws

BigInteger.Parse("1987442956125330092956022233482810406471227749906590072586420956431482462072838" + "\u0020\u0020\u0020", NumberStyles.AllowThousands);

this line doesn't throws but tests written so FormatException expected

[stephentoub]

I just tested this on both .NET 4.6 and with .NET Core:

BigInteger.Parse("1987442956125330092956022233482810406471227749906590072586420956431482462072838" + "\u0020\u0020\u0020", NumberStyles.AllowThousands);

and in both cases it throws a FormatException for me. You're seeing otherwise?

[Maxwe11]

@stephentoub test it with repro steps from #1491.
With en-US both .NET 4.5 and .NET Core throws FormatException as expected, but with Ukrainian format both .NET4.5 and .NET Core doesn't throws #1592 (comment)

[stephentoub]

Ah, ok, so this comment "Seems to me yes. I've checked same input with full .NET and it throws FormatException as expected" is no longer true, right? You're not currently aware of any inputs that have different exceptional behavior between .NET 4.6 and .NET Core? In which case, this is likely just a test bug?

[Maxwe11]

Ah, ok, so this comment "Seems to me yes. I've checked same input with full .NET and it throws FormatException as expected" is no longer true, right?

Right, I was wrong.
It's a test bug.

[stephentoub]

In that case, I suggest the [ActiveIssue] not be removed from the test, even if we want to change the culture references as this PR is doing. Or, should this PR just be closed out?

[Maxwe11]

@stephentoub I suggest to open new issue for this failure and put it number in [ActiveIssue].
CultureInfo changes were missed in #1206 because the test is [OuterLoop].
Seems to me strange that trailing spaces are culture dependent. Thoughts?

[stephentoub]

I suggest to open new issue for this failure and put it number in [ActiveIssue]

I'm fine with that. Can you do so and update your PR accordingly?

Seems to me strange that trailing spaces are culture dependent. Thoughts?

@krwq, @ellismg, or @tarekgh may have more thoughts? For reference, the issue being questioned is that with the following code:

static void Test(string culture)
{
    var oldCulture = CultureInfo.CurrentCulture;
    CultureInfo.CurrentCulture = new CultureInfo(culture);
    try
    {
        BigInteger.Parse(
            "1987442956125330092956022233482810406471227749906590072586420956431482462072838" + "\u0020\u0020\u0020",
            NumberStyles.AllowThousands);
    }
    finally { CultureInfo.CurrentCulture = oldCulture; }
}

if culture is "en-US", this throws a FormatException, but if culture is "uk-UA", it completes without error. That's true for both .NET Framework 4.6 and .NET Core. I assume this has something to do with the definition of whitespace in different cultures?

[Maxwe11]

I'm fine with that. Can you do so and update your PR accordingly?

done #1642

[stephentoub]

Thanks, LGTM

[tarekgh]

This test can reliably written by checking CurrentCulture.NumberFormat.NumberGroupSeparator if this property return space, then parsing will succeed otherwise it should throw. I didn't try it though so it is worth to try it.