add tests to increase enum serialization code coverage

[Jacksondr5]

Opening this PR because I messed up #39561, addressed feedback by @jozkee
Helps address #32341
Adds coverage to branch where WriteWithQuotes goes over the name cache soft limit. code
Adds coverage to branches where a Dictionary key is a numeric enum that is not in the cache and the value is not a valid identifier. code

[Dotnet-GitSync-Bot]

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.

[jozkee]

Thanks for the PR.
Could you please add cover for these few remaining lines as well? You can take a look at MoreThan64EnumValuesToSerialize test to have an idea on how to do so.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

Lines 357 to 360 in e0df256

	{
	// We also do not create a JsonEncodedText instance here because passing the string
	// directly to the writer is cheaper than creating one and not caching it for reuse.
	writer.WritePropertyName(original);

Otherwise, lgtm.

[Jacksondr5]

Thanks for the feedback! I looked at the lines you mentioned (357-360) and they appear to be covered already:

Is there something I'm missing here?

I did add a test for a branch that wasn't covered where the cache soft limit is hit with a naming policy provided. It's the false branch here:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

Lines 193 to 196 in 4bc91cb

	writer.WriteStringValue(
	_namingPolicy == null
	? original
	: FormatEnumValueToString(original, encoder));

[jozkee]

@Jacksondr5 that's odd, copying your changes locally, the lines aforementioned appear as uncovered on my end.

[ahsonkhan]

Is there something I'm missing here?

that's odd, copying your changes locally, the lines aforementioned appear as uncovered on my end.

How are you running the tests and measuring coverage (what command)? Are both of you running the tests in the same way (same architecture/runtime version/OS)? Maybe one of you has outerloop tests on.

[Jacksondr5]

I was running these using dotnet build /t:Test /p:Coverage=true /p:Outerloop=true and the branch is visited 2097088 times. Comparing the numbers on my output to the screenshot @jozkee provided, it does seem like outerloop could be the difference.

[jozkee]

@Jacksondr5 @ahsonkhan Ah, that's right, I was missing the OuterLoop argument. Since the lines that I pointed out are already covered by OuterLoop tests; this looks good to me as is.

These few lines are the only lines that are missing coverage on EnumConverter.cs:

For these yellow lines, I think we should instead address the comment given that #29000 is already closed.

[Jacksondr5]

Fixed the yellow lines. Since the coverage tool is currently acting weird with switch expressions, I blew the one below out into a bunch of if statements so that I could verify that all branches were being hit. I didn't commit those changes though, they were just for testing.

As for the unreached code in the constructor, I've got a question about that. To test that the cache limit works (ie doesn't throw an OutOfMemoryException), the other tests iterate over a flag enum and call Serialize on each iteration. I don't think we can do that with the constructor since it only caches the specifically defined values. Given that the other tests that cover the cache limit serialize up to 2097152 enums, I think we would need to have a hilariously large enum literal to truly test that the constructor code doesn't throw an OutOfMemoryException. Maybe I could do something with reflection, but I wanted to check before I wrote anything. Is there some other way to approach testing this code?

[jozkee]

As for the unreached code in the constructor, I've got a question about that. To test that the cache limit works (ie doesn't throw an OutOfMemoryException), the other tests iterate over a flag enum and call Serialize on each iteration

@Jacksondr5 you can cover those lines by just serializing an enum with 65 names, that will make the constructor to stop caching names at the soft limit (64). Now that I say that out loud, I think that maybe writing such test is not even worth it, we don't have to cover every single line just to strive to have as much lines covered as possible; 100% coverage in this file is already impossible due to unreachable code.

@layomia is there any value on covering the soft limit check in the constructor? if so, could you suggest a test for it?

Since the coverage tool is currently acting weird with switch expression, I blew the one below out into a bunch of if statements so that I could verify that all branches were being hit.

I'm glad that you confirmed that those lines are actually being covered but IMO I don't think we should make product changes (change source files) in PRs like this where the goal is add test coverage.

Fixed the yellow lines.

As I said, I think this PR should not contain product changes and we should open a separate PR that addresses the TODO comment.

[Jacksondr5]

Reverted all changes to EnumConverter.cs. I can open up a PR to address the TODO comment if desired.

[jozkee]

Otherwise; lgtm.

[jozkee]

I can open up a PR to address the TODO comment if desired.

@Jacksondr5 feel free to do so, thanks!

[ahsonkhan]

How long does this test end up taking (and roughly how much memory does it use up)?

[jozkee]

@Jacksondr5 Correct me if I'm wrong but I think it instantiates 2,097,152 (1 << 21) dictionaries.

Is there something that we could do to reduce this number while keeping the coverage? Perhaps you could do Serialize<MyEnum> until you have the cache at the desired number and then call Serialize<Dictionary<MyEnum, int>> on the desired edge cases.

[Jacksondr5]

I'll try my hand at benchmark.net and post some comparisons between VeryLargeAmountOfEnumsToSerialize and VeryLargeAmountOfEnumDictionaryKeysToSerialize so we can understand how long this takes and how serializing the Dictionary<MyEnum,int> compares to serializing MyEnums. Might take me a day or 2 depending on my day job

[ahsonkhan]

A rough estimate is sufficient (which probably doesn't need BDN, but feel free to go down that route).

If you run the tests locally from master, and then compare the test time of your branch (remove the outerloop from the attribute to try it), that should give you a general idea. I am looking for an order of magnitude. If the tests went from taking ~1 minute, to 2+, or if we are using up 100s of MB/GB of memory (on Windows, Task manager could show it), then the test is too resource intensive, and we would definitely keep it as outerloop, and try to make it simpler, if possible.

If not, then outerloop is good enough. I asked to understand whether we need this test to be outerloop or if it is fast enough to become part of innerloop.

@jozkee, can you help here and run the test on your local machine and see how much time this particular test is adding to the test run?

[jozkee]

Base time running dotnet build /t:test:

=== TEST EXECUTION SUMMARY ===
    System.Text.Json.Tests  Total: 8715, Errors: 0, Failed: 0, Skipped: 0, Time: 33.520s

Time running after adding this new test:

=== TEST EXECUTION SUMMARY ===
    System.Text.Json.Tests  Total: 8716, Errors: 0, Failed: 0, Skipped: 0, Time: 40.564s

@ahsonkhan I would consider keeping this test as OuterLoop considering that the test above, VeryLargeAmountOfEnumsToSerialize, which uses a similar code minus the insane amount of dictionary instances, is
already signaled to run on OuterLoop.

[ahsonkhan]

Time: 33.520s
Time: 40.564s

I would consider keeping this test as OuterLoop considering that the test above, VeryLargeAmountOfEnumsToSerialize, which uses a similar code minus the insane amount of dictionary instances, is already signaled to run on OuterLoop.

Agreed. That sounds good.