Help get System.Text.Json test coverage to 100% (or close to it)

[ahsonkhan]

Let's try to get the test coverage of all components of the JSON stack closer to 100%, where feasible.
We are in pretty good shape (well over 90%+). It tends to be much easier to maintain the bar once we hit 100% since any drop becomes clear/visible.

One component that is effectively at 100% is JsonElement. Let's see if we can get there for the rest.

That said, we shouldn't bend over backwards to try to get to 100% for things like testing all the conditions of a Debug.Asserts or return line after a throw. If some code is unreachable or not used, update/delete it.

Some test improvements are relatively easy to do, so I encourage folks who want to help contribute to System.Text.Json to start there. Others might require more work to bridge the test gap.

 +------------------+--------+--------+--------+
  | Module           | Line   | Branch | Method |
  +------------------+--------+--------+--------+
  | System.Text.Json | 94.04% | 91.22% | 98.05% |
  +------------------+--------+--------+--------+
  +---------+--------+--------+--------+
  |         | Line   | Branch | Method |
  +---------+--------+--------+--------+
  | Total   | 94.04% | 91.22% | 98.05% |
  +---------+--------+--------+--------+
  | Average | 94.04% | 91.22% | 98.05% |
  +---------+--------+--------+--------+

Here's our current JSON test coverage numbers for .NET Core (including outerloop which takes ~10 minutes to generate):
report.zip

Steps to generate:
Following the steps from https://github.com/dotnet/runtime/blob/master/docs/workflow/building/libraries/README.md#quick-start

1) build.cmd clr+libs -rc Release (this step takes ~10-20 minutes)
2) cd src\libraries\System.Text.Json\tests
3) If you want a quick report (~2 minutes), don't run the outerloop tests.
   a) dotnet build /t:Test /p:Coverage=true /p:BuildTargetFramework=net6.0
   b) dotnet build /t:Test /p:Coverage=true /p:BuildTargetFramework=net6.0 /p:Outerloop=true

Here are some good starting points:

1. JsonDocumentOptions
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocumentOptions.cs

   Line 12 in f5874b0

   public struct JsonDocumentOptions

• a)
  

2. JsonHelpers
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs

   Line 12 in f5874b0

   internal static partial class JsonHelpers

• a)
  
• b)
  
• c)
  
• d)
  

3. JsonClassInfo
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs

   Line 17 in f5874b0

   internal sealed partial class JsonClassInfo

• a)
  
• b)
  
• c)
  

4. JsonPropertyInfo
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs

   Line 14 in f5874b0

   internal abstract class JsonPropertyInfo

• a)
  

5. JsonPropertyInfoOfTTypeToConvert
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfTTypeToConvert.cs

   Line 14 in f5874b0

   internal sealed class JsonPropertyInfo<TTypeToConvert> : JsonPropertyInfo

• a)
  

6. JsonReaderHelper
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs

   Line 13 in f5874b0

   internal static partial class JsonReaderHelper

• a)
  

7. JsonSerializer
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs

   Line 10 in 527adf2

   public static partial class JsonSerializer

• a)
  
• b)
  
• c)
  
• d)
  
• e)
  
• f)
  

8. JsonSerializerOptions
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

   Line 15 in 527adf2

   public sealed partial class JsonSerializerOptions

• a)
  

9. ArrayConverter
   

   runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ArrayConverter.cs

   Line 14 in 527adf2

   internal sealed class ArrayConverter<TCollection, TElement>

• a)
  

10. ConcurrentStackOfTConverter
    

    runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs

    Line 11 in 527adf2

    internal sealed class ConcurrentStackOfTConverter<TCollection, TElement>

• a)
  

11. JsonConverterOfT.cs
    

    runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

    Line 14 in b95e523

    public abstract class JsonConverter<T> : JsonConverter

• a)
  
• b)
  
• c)
  

cc @jozkee, @layomia, @steveharter

[alanisaac]

I can pick up some of this work, so long as it'd be OK for a first contribution to the dotnet runtime. I've started on a couple in a draft PR (#32705), and will continue to work through them.

[alexvy86]

@ahsonkhan I started looking into this and ran into an issue running the tests, which I then traced to the fact that the RebuildAndTest target was recently removed. Maybe you want to update the commands in the description of this issue? (/t:RebuildAndTest becomes /t:Test)

   a) dotnet msbuild /t:Test /p:Coverage=true
   b) dotnet msbuild /t:Test /p:Coverage=true /p:Outerloop=true

[jozkee]

@alexvy86 done; thanks for pointing that out.

[ahsonkhan]

I started looking into this and ran into an issue running the tests

Do the build instructions need to be updated as well (step 1) or did that work as expected for you? I know there were some recent changes there too (cc @ViktorHofer).

[alexvy86]

The build worked fine, it was only running the tests that wasn't working.

[thall90]

I'm looking at testing portions of 2 (JsonHelpers) for a first-time contribution, and was wondering:
is it acceptable to expose internal classes for testing with something like [assembly: InternalsVisibleTo("System.Text.Json.Tests")], or should these classes be tested via outcomes of areas calling their methods?

[jozkee]

@thall90 My understanding is that we should avoid using InternalsVisibleTo on test projects as much as we can, I think there has also been efforts to remove its usage on several projects.
See related discussions:
dotnet/corefx#42879 (comment)
#14520 (comment)

[thall90]

Sounds good. Thanks!

[KimKiHyuk]

@thall90 you still work in progress on JsonHelpers?

if not, i hope to take it

[GMous]

It looks like the steps to generate coverage report need to be updated. Parameters "-subsetCategory" are no longer available. New version:
build.cmd -subset Clr -c Release && build.cmd -subset Libraries /p:CoreCLRConfiguration=Release

[ViktorHofer]

build.cmd clr+libs -c Release

[z77ma]

Hey guys. Wanted to get an actual coverage report, but the tests/report generating working sooo slow which confuses me a bit, I'm certain I missed smth and I'd appreciate an advice here. If it's important, then builds in both Debug and Release configuration are fine and take 'normal' time for such big code-base.

[ViktorHofer]

but the tests/report generating working sooo slow which confuses me a bit, I'm certain I missed smth and I'd appreciate an advice here.

hey @z77ma, what do you mean by slow? Do you have data to share?

[jozkee]

@z77ma @ViktorHofer it sounds like the runtime subset was built on Debug configuration.

@z77ma did you build using the suggested configuration?

build.cmd clr+libs -c Release

[ahsonkhan]

I updated the instructions in the original post of the issue:

Steps to generate:
Following the steps from https://github.com/dotnet/runtime/blob/master/docs/workflow/building/libraries/README.md#quick-start

1) build.cmd clr+libs -rc Release (this step takes ~10-20 minutes)
2) cd src\libraries\System.Text.Json\tests
3) If you want a quick report (~2 minutes), don't run the outerloop tests (which would take ~10 minutes).
   a) dotnet build /t:Test /p:Coverage=true /p:BuildTargetFramework=net6.0
   b) dotnet build /t:Test /p:Coverage=true /p:BuildTargetFramework=net6.0 /p:Outerloop=true

[z77ma]

Hey all.
@ViktorHofer, well I didn't preserve any results from that runs, but the longest attempt I had the patience to wait for lasted for approx. 3h and I killed the process afterward.
@jozkee, yes, I did use the suggested configuration. Basically, my question came as a result of an unexpected duration of a test run in Release configuration.
Anyway, today I tried to repeat all steps from scratch in order to provide more information and some statistics for you. And the issue seems to be gone. Though I can't completely explain what caused the described behavior. The things that differ from my previous attempt are: I build the whole runtime on Release configuration (build.cmd -c Release) and then I used the build command with the -rc flag, as @ahsonkhan suggested.
None of the above doesn't convince me to be a cure. So all looks like I messed smth with the configurations and run tests on Debug build.
I appreciate your help and suggestions. Cheers)