Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add tests to increase enum serialization code coverage #40601

Merged
merged 6 commits into from
Sep 23, 2020
Merged

add tests to increase enum serialization code coverage #40601

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b03a5a9
add tests to increase enum serialization code coverage
Jacksondr5 Aug 10, 2020
b3a2b9b
implement feedback and add test to cover naming policy scenario
Jacksondr5 Sep 12, 2020
0a6a345
remove unnecessary bound check now that all primitive types are suppo…
Jacksondr5 Sep 17, 2020
92f5a1b
Revert "remove unnecessary bound check now that all primitive types a…
Jacksondr5 Sep 17, 2020
bf62339
implement review feedback
Jacksondr5 Sep 18, 2020
1b4227e
Remove commented-out code
jozkee Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 231 additions & 1 deletion src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.IO;
using System.Threading.Tasks;
using Xunit;
Expand Down Expand Up @@ -185,7 +186,7 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer
private enum MyCustomEnum
{
First = 1,
Second =2
Second = 2
}

[Fact]
Expand Down Expand Up @@ -232,6 +233,23 @@ public static void MoreThan64EnumValuesToSerialize()
}
}

[Fact]
public static void MoreThan64EnumValuesToSerializeWithNamingPolicy()
{
var options = new JsonSerializerOptions
{
Converters = { new JsonStringEnumConverter(new ToLower()) }
Jacksondr5 marked this conversation as resolved.
Show resolved Hide resolved
};

for (int i = 0; i < 128; i++)
{
MyEnum value = (MyEnum)i;
string asStr = value.ToString().ToLowerInvariant();
string expected = char.IsLetter(asStr[0]) ? $@"""{asStr}""" : asStr;
Assert.Equal(expected, JsonSerializer.Serialize(value, options));
}
}
jozkee marked this conversation as resolved.
Show resolved Hide resolved

[Fact, OuterLoop]
public static void VeryLargeAmountOfEnumsToSerialize()
{
Expand Down Expand Up @@ -294,5 +312,217 @@ public enum MyEnum
U = 1 << 20,
V = 1 << 21,
}

[Fact, OuterLoop]
public static void VeryLargeAmountOfEnumDictionaryKeysToSerialize()
Comment on lines +316 to +317
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jozkee jozkee Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@Jacksondr5 Jacksondr5 Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ahsonkhan ahsonkhan Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
// Ensure we don't throw OutOfMemoryException.

const int MaxValue = (int)MyEnum.V;

// Every value between 0 and MaxValue maps to a valid enum
// identifier, and is a candidate to go into the name cache.

// Write the first 45 values.
Dictionary<MyEnum, int> dictionary;
for (int i = 1; i < 46; i++)
{
dictionary = new Dictionary<MyEnum, int> { { (MyEnum)i, i } };
JsonSerializer.Serialize(dictionary);
}

// At this point, there are 60 values in the name cache;
// 22 cached at warm-up, the rest in the above loop.

// Ensure the approximate size limit for the name cache (a concurrent dictionary) is honored.
// Use multiple threads to perhaps go over the soft limit of 64, but not by more than a couple.
Parallel.For(
0,
8,
i =>
{
dictionary = new Dictionary<MyEnum, int> { { (MyEnum)(46 + i), i } };
JsonSerializer.Serialize(dictionary);
}
);

// Write the remaining enum values. The cache is capped to avoid
// OutOfMemoryException due to having too many cached items.
for (int i = 54; i <= MaxValue; i++)
{
dictionary = new Dictionary<MyEnum, int> { { (MyEnum)i, i } };
JsonSerializer.Serialize(dictionary);
}
}

public abstract class NumericEnumKeyDictionaryBase<T>
{
public abstract Dictionary<T, int> BuildDictionary(int i);

[Fact]
public void SerilizeDictionaryWhenCacheIsFull()
{
var options = new JsonSerializerOptions
{
Converters = { new JsonStringEnumConverter() }
};
Jacksondr5 marked this conversation as resolved.
Show resolved Hide resolved

Dictionary<T, int> dictionary;
for (int i = 1; i <= 64; i++)
{
dictionary = BuildDictionary(i);
JsonSerializer.Serialize(dictionary, options);
}

dictionary = BuildDictionary(0);
string json = JsonSerializer.Serialize(dictionary, options);
Assert.Equal($"{{\"0\":0}}", json);
}
}

public class Int32EnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumInt32>
{
public override Dictionary<SampleEnumInt32, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumInt32, int> { { (SampleEnumInt32)i, i } };
}

public class UInt32EnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumUInt32>
{
public override Dictionary<SampleEnumUInt32, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumUInt32, int> { { (SampleEnumUInt32)i, i } };
}

public class UInt64EnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumUInt64>
{
public override Dictionary<SampleEnumUInt64, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumUInt64, int> { { (SampleEnumUInt64)i, i } };
}

public class Int64EnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumInt64>
{
public override Dictionary<SampleEnumInt64, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumInt64, int> { { (SampleEnumInt64)i, i } };
}

public class Int16EnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumInt16>
{
public override Dictionary<SampleEnumInt16, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumInt16, int> { { (SampleEnumInt16)i, i } };
}

public class UInt16EnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumUInt16>
{
public override Dictionary<SampleEnumUInt16, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumUInt16, int> { { (SampleEnumUInt16)i, i } };
}

public class ByteEnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumByte>
{
public override Dictionary<SampleEnumByte, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumByte, int> { { (SampleEnumByte)i, i } };
}

public class SByteEnumDictionary : NumericEnumKeyDictionaryBase<SampleEnumSByte>
{
public override Dictionary<SampleEnumSByte, int> BuildDictionary(int i) =>
new Dictionary<SampleEnumSByte, int> { { (SampleEnumSByte)i, i } };
}


[Flags]
public enum SampleEnumInt32
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumUInt32 : uint
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumUInt64 : ulong
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumInt64 : long
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumInt16 : short
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumUInt16 : ushort
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumByte : byte
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}

[Flags]
public enum SampleEnumSByte : sbyte
{
A = 1 << 0,
B = 1 << 1,
C = 1 << 2,
D = 1 << 3,
E = 1 << 4,
F = 1 << 5,
G = 1 << 6,
}
}
}