Runtime: Improve test coverage for JsonElement (when backed by JsonNode) for edge cases around enumeration

[ahsonkhan]

We can improve test coverage of certain branches within JsonElement (when it's backed by JsonNode):

Metric | Value
-- | --
Covered lines: | 629
Uncovered lines: | 13
Coverable lines: | 642
Total lines: | 2096
Line coverage: | 97.9% (629 of 642)
Covered branches: | 213
Total branches: | 225
Branch coverage: | 94.6% (213 of 225)

Edge cases while enumerating JSON objects and JSON arrays. It's possible these branches are unreachable:
https://github.com/dotnet/corefx/blob/e8efb815275c8323fedc950ca3596474fa5e23c3/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.ArrayEnumerator.cs#L51-L59

https://github.com/dotnet/corefx/blob/e8efb815275c8323fedc950ca3596474fa5e23c3/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.ObjectEnumerator.cs#L47-L55

These tests won't fail on .NET Core (and they are likely already covered by tests that run on netfx, but it would be good to verify).
https://github.com/dotnet/corefx/blob/e8efb815275c8323fedc950ca3596474fa5e23c3/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L1097-L1105

https://github.com/dotnet/corefx/blob/e8efb815275c8323fedc950ca3596474fa5e23c3/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L1176-L1184

cc @kasiabulat

[AntonLandor]

Hi,

I just started looking into contributing to this project as part of my advanced course in Large Scale Software Development. I figured I would start with this issue. I just have a few questions:

1. Can I easily check which branches are covered and which are not? If so, how do I do it? I'm currently on a machine running Ubuntu and I use Visual Studio Code.
2. Am I correct that tests regarding this issue are located in corefx/src/System.Text.Json/tests/JsonElementWithNodeParentTests.cs?

[MarcoRossignoli]

Can I easily check which branches are covered and which are not? If so, how do I do it? I'm currently on a machine running Ubuntu and I use Visual Studio Code.

Yes usually html report is generated from coverage file(xml or other format, for instance cobertura,lcov,opencover) you can parse that file by yourself and find where is the uncovered branches.
Format here should be opencover https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.CoreFxTesting/build/core/coverage/Coverage.props#L9 cc: @ViktorHofer

[kasiabulat]

As far as I remember I might have already covered some of those in https://github.com/dotnet/corefx/pull/41041, but let me check it.

[kasiabulat]

Ok, right, those are uncovered, but I treated them as unreachable.

[ViktorHofer]

Can I easily check which branches are covered and which are not? If so, how do I do it? I'm currently on a machine running Ubuntu and I use Visual Studio Code.

Yes when do a dotnet msbuild /t:RebuildAndTest /p:Coverage=true it generates both a coverage.xml and a index.htm file.

I just started looking into contributing to this project as part of my advanced course in Large Scale Software Development. I figured I would start with this issue. I just have a few questions:

Fantastic, great to hear that you want to help us. cc @danmosemsft as he might have some other items as well to pick.

[danmosemsft]

@AntonLandor as @ViktorHofer says, we welcome new contributors! We can help find you work that may be interesting to you. System.Text.JSON is a great place to help as it is new this release and there are many improvements proposed.. You can also use @ahsonkhan as a resource for this.

[ahsonkhan]

Ok, right, those are uncovered, but I treated them as unreachable.

The uncovered branches in JsonElement.ArrayEnumerator.cs and JsonElement.ObjectEnumerator.cs are reachable and hence need tests. As an example, a test like the following should cover one of them (right after MoveNext returns false):
C# Assert.Equal(default, arrayEnumerator.Current);
In here:
https://github.com/dotnet/corefx/blob/4ca1feeeb484e8a7089ce8a9d377703ad5b8a53e/src/System.Text.Json/tests/JsonElementWithNodeParentTests.cs#L280-L284

Can I easily check which branches are covered and which are not? If so, how do I do it? I'm currently on a machine running Ubuntu and I use Visual Studio Code.

As @ViktorHofer mentioned,
1) Build corefx at the root of the repo (see https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md)
2) Navigate to the test directory for this project: cd src\System.Text.Json\tests
3) Run dotnet msbuild /t:RebuildAndTest /p:Coverage=true
4) Open up and see the coverage report. You can find that in:
<repo root>\artifacts\bin\System.Text.Json.Tests\netcoreapp-Debug\report\index.htm

The screenshots I shared in the issue are from that coverage report itself.

Am I correct that tests regarding this issue are located in corefx/src/System.Text.Json/tests/JsonElementWithNodeParentTests.cs?

Yes.

You can also use @ahsonkhan as a resource for this.

Absolutely. Let me know if you have any questions or get blocked on something.

[AntonLandor]

Thanks for all the great input! I'll start looking into this issue. I have yet to get familiar with the basic process of developing for CoreFX, and I'll let you know if there's any trouble along the way. The Developer Guide seems really helpful, I missed it when first reading up on the project. Thanks!

[AntonLandor]

Alright, I think I've covered all the reachable branches. Though I haven't been able to get https://github.com/dotnet/corefx/blob/e8efb815275c8323fedc950ca3596474fa5e23c3/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L1176-L1184
and https://github.com/dotnet/corefx/blob/e8efb815275c8323fedc950ca3596474fa5e23c3/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L1097-L1105
to throw a FormatException each. On GetSingle() I've tried
Assert.Throws<FormatException>(() => new JsonNumber(Single.MaxValue + 1).AsJsonElement().GetSingle());,
Assert.Throws<FormatException>(() => new JsonNumber(Double.MaxValue).AsJsonElement().GetSingle()); and
Assert.Throws<FormatException>(() => new JsonNumber().AsJsonElement().GetSingle());
among others. I will not get a FormatException on any of these though, and I would guess there's an explicit conversion being done which explains why these wont fail even though no Single was passed as values. I'm having pretty much the same problem with GetDouble(). To get a FormatException I need the JsonElement to be a JsonNumber, which I can't construct without it being convertible to a Single and Double to my knowledge. @ahsonkhan should I treat these branches as unreachable and make a pull request with all the other branches covered?

[ahsonkhan]

make a pull request with all the other branches covered?

Yes, please.

should I treat these branches as unreachable

I've tried
Assert.Throws<FormatException>(() => new JsonNumber(Single.MaxValue + 1).AsJsonElement().GetSingle());,

I am not sure if that approach would work. Keep in mind that on .NET Core (which is the default platform when you build corefx),TryGetDouble will never return false (becausedouble.TryParse won't return false). See this remark:
https://github.com/dotnet/corefx/blob/5bc2806f33090e78b38fafe4d5b46d5a0a4c1f08/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L1037-L1042

What you need to do is try testing on netfx and see if we already have tests covering that branch. If not, add such tests. Try your approach on netfx and see what happens.

Alternatively, you could create a JsonNumber from a really large string that isn't representable by a float. Then, calling TryGetSingle on that should return false (on netfx, rather than returning float.PositiveInfinity), and you can exercise the "throw" branch. Also, keep in mind, that such a test would need to have framework specific asserts (throw on netfx, but work and return float.PositiveInfinity on net core). So, maybe something like this (and the same for double):
C# Assert.Throws<FormatException>(() => new JsonNumber("3.40282347E+39").AsJsonElement().GetSingle());

Use the -framework option in the build command to build/test for netfx (see https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#build)

[safern]

@ahsonkhan should this be closed per: dotnet/corefx#41252 or the Writable Json DOM item is pending?

[ahsonkhan]

I didn't hear back about https://github.com/dotnet/corefx/pull/41252#issuecomment-534794724, but I can double-check that myself and then we can close this issue.

[AntonLandor]

@ahsonkhan sorry I was busy with other studies this week so I haven't had time to work on this project. Great that you can solve it!