Updated tests regarding issue #48119

[ayousuf23]

Added more tests cases for parsing floating-point types (single, double, half, decimal). Related to issue #48119. Note that this PR request is for checking why an "AmbigiousMatchException" is thrown when running the tests.

[ghost]

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.

[dnfadmin]

All CLA requirements met.

[ayousuf23]

@pgovind Here is my pull request regarding issue #48119.

[ghost]

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Added more tests cases for parsing floating-point types (single, double, half, decimal). Related to issue #48119. Note that this PR request is for checking why an "AmbigiousMatchException" is thrown when running the tests.

Author:	ayousuf23
Assignees:	-
Labels:	area-System.Runtime
Milestone:	-

[danmoseley]

@ayousuf23 I apologize that we did not review this one promptly. It does not normally take this long. I left one comment otherwise it looks good once you resolve the conflicts.

[pgovind]

I think you should replace Decimal.Parse(inputForDecimalWithDotAtMaxDigit) with 61111...111.m. That is because this unit test is called by the Parse method here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/DecimalTests.cs#L814. If you look at the implementation of that method, it calls decimal.TryParse(value) and check if the returned out result is equal to expected on the next line (Line 824). With

yield return new object[] { inputForDecimalWithDotAtMaxDigit, defaultStyle, null, Decimal.Parse(inputForDecimalWithDotAtMaxDigit) };

value will be inputForDecimalWithDotAtMaxDigit and expected will be Decimal.Parse(inputForDecimalWithDotAtMaxDigit). So, we will end up checking Assert.Equal(Decimal.Parse(inputForDecimalWithDotAtMaxDigit), Decimal.Parse(inputForDecimalWithDotAtMaxDigit)) which means that we are not really testing result with a known correct expected value. So, the way to fix this is to specify expected directly as a decimal value. That way we actually test the Parse implementation.

Similar comments for double, single and half!

[pgovind]

The test cases themselves are good. Just needs 3 things:

1. Fix the comment I left so we actually test the Parse routine
2. Remove the changes to the sln file.
3. You should rebase this PR on top of the current dotnet:main (or merge dotnet:main in to your branch) so you don't have merge conflicts anymore. Let me know if you need help here!

[ayousuf23]

@pgovind I updated the test cases and removed the change in the sln file (this was caused by Visual Studio).

[pgovind]

I updated the test cases and removed the change in the sln file

Are you sure you pushed your latest commit? I still see the changes to the sln file.

This PR is almost ready. If you could just revert the changes to the sln file, I will approve it!!

[jeffhandley]

@pgovind This PR is assigned to you for follow-up/decision before the RC1 snap.

[ayousuf23]

I'm so sorry, I didn't see your comment until now. I will check the sln file to ensure the line added by Visual Studio was removed.

[ayousuf23]

@pgovind I removed the lines from the sln file. It should be good to go!

[pgovind]

Still seeing changes to Runtime.sln. If you revert them so it doesn't even appear in the Files Changes tab, I can approve.

[pgovind]

I tried to clean up your commits, but it looks like you merged master into your branch? So there's a lot of history. If you find it hard to revert the changes to Runtime.sln, my suggestion is to just create a new branch off of current upstream/main and copy only the changes to the *.Tests.cs file.

[ayousuf23]

@pgovind Thanks for the suggestion! I copied the changes to the tests and reset the branch to dotnet/main and then added the changes to the tests. Sorry for the trouble. The file changed tab now shows only the lines added to the files for decimal/double/single/half.

[pgovind]

Weird, CI is failing now @ayousuf23.

[ayousuf23]

I tried to adjust the values for the string length for testing parsing a string with '.' at or after half/single max digit count. I'm just trying to see if the checks pass now.

[ayousuf23]

@pgovind I'm not sure why the checks are failing.

[ayousuf23]

@pgovind I looked through the checks, and it seems like the tests are failing to parse ".234" which is expected to have the value 0.234.

[pgovind]

The test changes LGTM. I do see a lot of weird CI errors though. I'll rerun CI and see if they reproduce. Thanks for the great work and patience here @ayousuf23

[tannergooding]

@ayousuf23 it looks like there is at least one legitimate failure Test System.Tests.HalfTests.Parse_Span_Valid has failed.

Could you please take a look and address this or let us know if you need assistance? Doing a merge with upstream/main would likely also help with a few things in CI.

[jeffhandley]

Failures are unrelated, as noted by the runfoapp bot.

[ayousuf23]

@jeffhandley Thanks for the commit! @tannergooding I am sorry I did not respond earlier. Unfortunately, I do not have access to the computer I used previously to write the code and work on the project.

[jeffhandley]

No worries, @ayousuf23; thanks for the contribution. It'll be nice to get this merged. 🙂

[ayousuf23]

Thank you everyone for your help! This was my first PR and I learned a lot from it!

[danmoseley]

@ayousuf23 thank you, and you would be welcome to offer another contribution if you are interested! There are lots of issues, many marked-up-for-grabs, and we can help find something matching your interests.