More Parse tests for double, single and Half #50394

pgovind · 2021-03-29T23:10:48Z

Fixes a part of #48119 (specifically point 5 in that issue: More nuanced floating point numbers that involve trailing zeros, rounding and precision concerns.)

ghost · 2021-03-29T23:10:52Z

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

Issue Details

Fixes a part of #48119 (specifically point 5 in that issue: More nuanced floating point numbers that involve trailing zeros, rounding and precision concerns.)

Author:	pgovind
Assignees:	-
Labels:	`area-System.Numerics`
Milestone:	-

src/libraries/System.Runtime/tests/System/DoubleTests.cs

jeffhandley

I don't expect so, but it's worth asking: do these tests impact build/test times noticeably? It's a lot of scenarios being tested.

jeffhandley · 2021-03-30T21:15:11Z

src/libraries/System.Runtime/tests/System/DoubleTests.cs

+ return new string(charArray);
+ }
+
+ [Fact]


I often advise against having Assert inside a loop as the test will then just fail at the first occurrence instead of showing how many cases are affected. This might be a good candidate for using [ClassData] to dynamically test all of the loaded scenarios. You could then have the data in a convenient structure too.

I will consider this on my next pass through here. I actually tried putting the data from ibm-fpgen.txt into a static class to call with ClassData like you suggest, but VS turned out to be so sluggish once the file got big that it was impossible to work on that file. I'm wondering if design time build in VS was the culprit.

Ha - I bet. You could also just have a class that reads the data from the text file and loads it into a well-formed structure though.

You could do

[Theory] [MemberData(nameof(ParsePattern_TestData))]

with

public static IEnumerable<object[]> ParsePattern_TestData() { string path = Directory.GetCurrentDirectory(); using (FileStream file = new FileStream(Path.Combine(path, "NumericsTestData", "ibm-fpgen.txt"), FileMode.Open)) { // logic yield return doubleBytes; } }

Yeah, I too was trying to suggest something like that where the data remains in a text file (not intended to be opened in Visual Studio), but that was loaded into a class/member instead of parsed as part of the test itself. It's OK with me to have that done in a follow-up PR though.

pgovind · 2021-04-01T21:28:06Z

I don't expect so, but it's worth asking: do these tests impact build/test times noticeably? It's a lot of scenarios being tested.

I had the same thought too. As it turns out, it only takes ~1s to execute all 3 tests.

pgovind · 2021-04-16T18:54:40Z

src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj

@@ -272,6 +272,9 @@
 <Compile Include="$(CommonTestPath)System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs"
 Link="Common\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs" />
 </ItemGroup>
+ <ItemGroup>
+ <None Include="NumericsTestData\ibm-fpgen.txt" CopyToOutputDirectory="Always" />


@Anipik @ViktorHofer

Are we ok with checking in test data like this? Or ss there an existing way to add test data like I'm doing here? I don't see a "CommonTestData" or something similar in the repo.

we generally use https:/dotnet/runtime-assets for larger data. As its a test only change, we can remove it next preview if need be

Depends on how large the test data is? If it's less than 500KB I would say, check it in. If larger I would recommend to check it into https:/dotnet/runtime-assets. Note that this isn't reversible as the BLOB will stay in the git history of the repo forever.

Ah perfect, the 500kb guideline was what I was looking for. Thanks! Will push this to runtime-assets first then.

That's less a guideline but more a recommendation from my side. It also depends on how git versions the file as it treats text files vs binary blobs differently.

jeffhandley · 2021-04-20T17:41:04Z

At some point, @pgovind, I saw you had a commit that used [MemberData]; that commit seems to be gone though. I had suggested doing it in a follow-up; was that your plan?

pgovind · 2021-04-20T17:49:21Z

At some point, @pgovind, I saw you had a commit that used [MemberData]; that commit seems to be gone though. I had suggested doing it in a follow-up; was that your plan

Indeed. With [MemberData], I had the following problem:

 [MemberData(nameof(MethodThatYieldedData))]
 public void DoubleParseTest () { actual test }

 [MemberData(nameof(MethodThatYieldedData))]
 public void SingleParseTest () { actual test }

 [MemberData(nameof(MethodThatYieldedData))]
 public void HalfParseTest () { actual test }



public static void MethodThatYieldedData
{
Code that opens the input file and yields data
}

The tests would fail because all 3 *ParseTest methods would try to open the file and only 1 of them would succeed. The other 2 would fail because the file was already open. So I just moved everything to a single test for now. On my next pass through here, I will split them out to 3 different tests.

jeffhandley · 2021-04-20T17:56:42Z

Ah, gotcha; thanks. You'd probably need to move the file parsing into the test class constructor then, which is treated as a setup method and should only execute once.

pgovind · 2021-05-25T21:58:44Z

Just adding a note for myself that this PR should go in as 2 pieces of work:

A PR in runtime-assets to get the test data in first
The unit tests defined in this PR can then go in

eng/Version.Details.xml

pgovind · 2021-06-21T22:37:13Z

src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj

@@ -282,6 +282,7 @@
 </ItemGroup>
 <ItemGroup>
 <PackageReference Include="Moq" Version="$(MoqVersion)" />
+ <PackageReference Include="System.Runtime.Numerics.TestData" Version="$(SystemRuntimeNumericsTestDataVersion)" GeneratePathProperty="true"/>


Just out of curiosity, what does the GeneratePathProperty do? @safern

pgovind · 2021-06-21T22:40:37Z

Alright, I updated this branch to use the assets produced by https:/dotnet/runtime-assets/pull/143/files. There were no other changes here, so I'm just going to re-use the prior approvals I already had. Just calling it out for @jeffhandley and @tannergooding. I'll merge if CI is green. I don't expect any new comments, but there are any, I'll address it in a follow up PR.

ghost · 2021-06-21T22:41:00Z

Hello @pgovind!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ViktorHofer · 2021-06-22T13:36:11Z

eng/Version.Details.xml

@@ -214,5 +214,9 @@
 <Uri>https:/dotnet/hotreload-utils</Uri>
 <Sha>25b814e010cd4796cedfbcce72a274c26928f496</Sha>
 </Dependency>
+ <Dependency Name="System.Runtime.Numerics.TestData" Version="6.0.0-beta.21314.1">
+ <Uri>https:/dotnet/runtime-assets


Just to let you know, this broke dependency flow as you were missing the </Uri> element. I'm fixing this in #54511.

Ah sorry my bad. Thanks for fixing it!

uweigand · 2021-06-28T11:41:54Z

This test is failing on big-endian systems, see #54818 for a fix. Thanks!

pgovind added the area-System.Numerics label Mar 29, 2021

pgovind requested a review from tannergooding March 29, 2021 23:10

pgovind commented Mar 29, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/DoubleTests.cs Show resolved Hide resolved

jeffhandley requested changes Mar 30, 2021

View reviewed changes

pgovind commented Apr 16, 2021

View reviewed changes

tannergooding approved these changes Apr 16, 2021

View reviewed changes

jeffhandley approved these changes Apr 16, 2021

View reviewed changes

pgovind force-pushed the numerics_parse_tests branch 2 times, most recently from 180ac83 to a2d686d Compare April 19, 2021 18:10

pgovind added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 20, 2021

jeffhandley marked this pull request as draft May 9, 2021 05:17

pgovind force-pushed the numerics_parse_tests branch from 6fd2bec to 23d6469 Compare June 21, 2021 21:19

pgovind commented Jun 21, 2021

View reviewed changes

eng/Version.Details.xml Show resolved Hide resolved

Prashanth Govindarajan added 2 commits June 21, 2021 15:32

Test ibm-fpgen locally for validation

8381cfd

sq

1be977d

pgovind force-pushed the numerics_parse_tests branch from 23d6469 to 1be977d Compare June 21, 2021 22:36

pgovind commented Jun 21, 2021

View reviewed changes

pgovind marked this pull request as ready for review June 21, 2021 22:38

pgovind added auto-merge and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Jun 21, 2021

ghost merged commit bea8d95 into dotnet:main Jun 22, 2021

ViktorHofer reviewed Jun 22, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Jul 28, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More Parse tests for double, single and Half #50394

More Parse tests for double, single and Half #50394

pgovind commented Mar 29, 2021

ghost commented Mar 29, 2021

jeffhandley left a comment

jeffhandley Mar 30, 2021

pgovind Apr 1, 2021 •

edited

Loading

jeffhandley Apr 1, 2021

tannergooding Apr 16, 2021

jeffhandley Apr 16, 2021

pgovind commented Apr 1, 2021

pgovind Apr 16, 2021

Anipik Apr 16, 2021 •

edited

Loading

ViktorHofer Apr 20, 2021

pgovind Apr 20, 2021

ViktorHofer Apr 20, 2021

jeffhandley commented Apr 20, 2021

pgovind commented Apr 20, 2021

jeffhandley commented Apr 20, 2021

pgovind commented May 25, 2021

pgovind Jun 21, 2021 •

edited

Loading

pgovind commented Jun 21, 2021

ghost commented Jun 21, 2021

ViktorHofer Jun 22, 2021

pgovind Jun 22, 2021

uweigand commented Jun 28, 2021

More Parse tests for double, single and Half #50394

More Parse tests for double, single and Half #50394

Conversation

pgovind commented Mar 29, 2021

ghost commented Mar 29, 2021

jeffhandley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgovind Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgovind commented Apr 1, 2021

Choose a reason for hiding this comment

Anipik Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffhandley commented Apr 20, 2021

pgovind commented Apr 20, 2021

jeffhandley commented Apr 20, 2021

pgovind commented May 25, 2021

pgovind Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

pgovind commented Jun 21, 2021

ghost commented Jun 21, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uweigand commented Jun 28, 2021

pgovind Apr 1, 2021 •

edited

Loading

Anipik Apr 16, 2021 •

edited

Loading

pgovind Jun 21, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.