[release/6.0-preview2] Track trailing zeros only for floating point numbers #48857
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @tannergooding, @pgovind Issue DetailsBackport of #48608 to release/6.0-preview2 /cc @jeffhandley @pgovind @tannergooding Customer ImpactFixes #48604 and #48648. #47666 introduced a regression in the parsing routine and made it to the P2 snap. This PR fixes that regression. TestingASP.NET also found the regression in their testing of the P2 bits. I got sign-off from ASP.NET for the fix from @pranavkm. I've also added new unit tests to test our parsing logic better. RiskMinimal. We're broken already, and this fixes the break. I've also reduced the scope of the change to only affect parsing doubles.
|
|
Could you please include in the template why we should take the change. I assume in this case we think the impact is widespread. Did you get a chance to run your new changes against all the inputs in https:/nigeltao/parse-number-fxx-test-data/tree/main/data as suggested in #48648? |
| @@ -1500,6 +1500,14 @@ public static void ToString_InvalidFormat_ThrowsFormatException() | |||
| Assert.Throws<FormatException>(() => f.ToString("E" + intMaxPlus1String)); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [InlineData("3.00")] | |||
There was a problem hiding this comment.
Aren't there a bunch of other possible inputs here -- or conversely, should this just be a case added to an existing test?
There was a problem hiding this comment.
This specific scenario is the one that ASP.NET reported as broken, FYI.
There was a problem hiding this comment.
Aren't there a bunch of other possible inputs here
There are. The plan is to have a working group some time soon to improve our unit tests for the parsing and formatting routines. I expect that this unit test will be folded into whatever we create at that point OR we'll add those inputs to this unit test.
Yup, the impact is widespread. 2 common cases that are broken with the current P2 bits are: int.Parse("3.00", options) throws an exception. This is fixed by this PR
string->decimal->string roundtripping is also broken. ASP.NET hit this issue in their testing of the current P2 bits. This is also fixed by this PR. |
|
OK, seems reasonable for Preview 2. |
Not yet. This is part of the plan for the working group. Though I guess I can run this locally right now while waiting for the build. I'll report what I find here. |
|
Ok, I can confirm that all the tests mentioned in that issue ( |
|
Ok thanks. Might be worth a check at some point that we have 100% code coverage of this stuff but not suggesting it's necessary for this change. |
|
Test failures are unrelated. Merging (I assume I can. If not, it should be easy enough to revert). |
|
Oh apparently, I need someone to officially approve the PR. Alright, whoever approves the PR, please merge it too! |
|
@pgovind @danmoseley I don't have permission to merge; I'm not sure who does. I just fired off a re-run of the checks that had failed though. |
|
Any of us should be able to merge |
|
Weird, I see a message that says “you’re not authorized to merge this PR” |
|
I'm assuming it's because the CI is red and the branch protection is blocking. We'll see if re-running these legs gets it green. We can check in on it again tomorrow. |
|
Oh wait I remember stephentoub commenting somewhere that we disabled some dns tests in runtime recently. I wonder if these are the same failures. Will check if this is the case later today |
|
Merge worked for me. Failures were #48751 |
|
Thanks, @danmoseley! |
ghost
locked as resolved and limited conversation to collaborators
Backport of #48608 to release/6.0-preview2
/cc @jeffhandley @pgovind @tannergooding
Customer Impact
Fixes #48604 and #48648. #47666 introduced a regression in the parsing routine and made it to the P2 snap. This PR fixes that regression.
This issue is widespread; 2 common cases that are broken with the current P2 bits are:
int.Parse("3.00", options)throws an exception. This is fixed by this PR.string->decimal->stringroundtripping is also broken. ASP.NET hit this issue in their testing of the current P2 bits. This is also fixed by this PR.Testing
ASP.NET also found the regression in their testing of the P2 bits. I got sign-off from ASP.NET for the fix from @pranavkm. I've also added new unit tests to test our parsing logic better.
As we investigated and resolved this, we concluded that our test coverage of number formatting/parsing is generally insufficient. We will schedule work later in the release to introduce significantly more comprehensive test coverage.
Risk
Minimal. We're broken already, and this fixes the break. I've also reduced the scope of the change to only affect parsing floating point types.