Additional code coverage in System.Text.Json #33019

alanisaac · 2020-02-29T23:22:34Z

Note 1

In order to get coverage on the following lines, I added a missing test case using JsonDocument.Parse with a large, invalid base64 string without an escaped character. The test data previously used [InlineData(...)] attributes where the large strings were generated from the [InlineData(null)] case, using null as a sentinel value. I weighed a few different options for how to add this new case, and eventually settled on moving the test data to its own class and using [MemberData(...)] instead to simplify the tests themselves. Certainly open to suggestions!

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

Lines 162 to 166 in 11b37df

    
           if (pooledArray != null) 
        
           { 
        
               byteSpan.Clear(); 
        
               ArrayPool<byte>.Shared.Return(pooledArray); 
        
           }

Note 2
There was a missing case in cyclic tests that validates that EffectiveMaxDepth is set to 64 when MaxDepth is set to 0 (lines). I ended up splitting WriteCyclicFail, which had two logical subgroups into WriteCyclic and WriteCyclicFail.

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

Line 203 in 527adf2

EffectiveMaxDepth = (value == 0 ? JsonReaderOptions.DefaultMaxDepth : value);

ahsonkhan

Some nits. But looks great.

ahsonkhan · 2020-03-02T18:54:14Z

src/libraries/System.Text.Json/tests/JsonBase64TestData.cs

@@ -0,0 +1,61 @@
+using System.Collections.Generic;


This needs a licence header comment.

fixed in 6c8ca54

ahsonkhan · 2020-03-02T18:55:29Z

src/libraries/System.Text.Json/tests/JsonBase64TestData.cs

+            yield return new object[] { "\"ABC===\"" };
+            yield return new object[] { "\"ABC\"" };
+            yield return new object[] { "\"ABC!\"" };
+            yield return new object[] { GenerateRandomInvalidLargeString(true) };


nit: use named parameter to help with readability

Suggested change

yield return new object[] { GenerateRandomInvalidLargeString(true) };

yield return new object[] { GenerateRandomInvalidLargeString(includeEscapedCharacter: true) };

fixed in e4ba4ce

ahsonkhan · 2020-03-02T18:56:11Z

src/libraries/System.Text.Json/tests/JsonBase64TestData.cs

+        private static string GenerateRandomValidLargeString()
+        {
+            var random = new Random(42);
+            var charArray = new char[502];


nit: I know you copied this from existing tests, but why did we pick 502 as the size? Consider adding a comment.

added explanation in b9b42e1

ahsonkhan · 2020-03-02T18:56:43Z

src/libraries/System.Text.Json/tests/JsonBase64TestData.cs

+            var random = new Random(42);
+            var charArray = new char[502];
+            charArray[0] = '"';
+            for (int i = 1; i < charArray.Length; i++)


If we are going to write " at the end anyway, reduce index by one?

Suggested change

for (int i = 1; i < charArray.Length; i++)

for (int i = 1; i < charArray.Length - 1; i++)

fixed in d09874d

ahsonkhan · 2020-03-02T19:03:40Z

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Deserialize.cs

+        [InlineData(@"{""$iz"": ""1""}")]
+        [InlineData(@"{""$rez"": ""1""}")]
+        [InlineData(@"{""$valuez"": []}")]
+        public static void InvalidMetadataPropertyNameWithSameLengthIsNotRecognized(string json)


ahsonkhan · 2020-03-02T19:04:19Z

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Deserialize.cs

+        [InlineData(@"{""$valuez"": []}")]
+        public static void InvalidMetadataPropertyNameWithSameLengthIsNotRecognized(string json)
+        {
+            JsonException ex = Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Employee>(json, s_deserializerOptionsPreserve));


Do we want to assert anything about the returned JsonException itself and its properties?
If not:

Suggested change

JsonException ex = Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Employee>(json, s_deserializerOptionsPreserve));

Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Employee>(json, s_deserializerOptionsPreserve));

Good point -- added validation of the expected path in 9c0af54

ahsonkhan · 2020-03-02T19:06:43Z

src/libraries/System.Text.Json/tests/Serialization/CyclicTests.cs

+        [InlineData(2, 3)]
+        [InlineData(10, 11)]
+        [InlineData(70, 71)]
+        [InlineData(10, 0)]


nit: Add a comment here that 0 (or default) max depth is treated as 64. Something like this (feel free to reword):

Suggested change

[InlineData(10, 0)]

[InlineData(10, 0)] // 0 (or default) max depth is treated as 64.

fixed in 70835c4

ahsonkhan · 2020-03-02T19:07:02Z

src/libraries/System.Text.Json/tests/Serialization/CyclicTests.cs

+        [InlineData(2, 2, 1)]
+        [InlineData(10, 10, 9)]
+        [InlineData(70, 70, 69)]
+        [InlineData(70, 0, 63)]


Same nit: comment that 0 is special and means 64.

fixed in 70835c4

alanisaac · 2020-03-03T00:31:40Z

@ahsonkhan I found that there was another set of tests for Base64 strings using the exact same test data. I thought it would be a good idea to reuse it to reduce LoC, similar to other data sets like ValidISO8601Tests. See 8c893c0

layomia

Thanks!

alanisaac added 3 commits February 29, 2020 15:45

added tests for invalid metadata properties with same length

3ccdb58

moved base64 test data in class, added test case

3b4df30

added missing cyclic test for max depth of 0

81bfd21

alanisaac requested review from ahsonkhan, jozkee, layomia and steveharter as code owners February 29, 2020 23:22

Dotnet-GitSync-Bot added the area-System.Text.Json label Feb 29, 2020

added test for null converter

54eb278

layomia added this to the 5.0 milestone Mar 2, 2020

layomia added the test-enhancement Improvements of test source code label Mar 2, 2020

ahsonkhan approved these changes Mar 2, 2020

View reviewed changes

alanisaac added 5 commits March 2, 2020 19:17

validated exception path

9c0af54

added comment explaining 0 max depth

70835c4

using named parameter for readability

e4ba4ce

added license header

6c8ca54

replacing duplicate test data

8c893c0

removed redunant character assignment

d09874d

ahsonkhan approved these changes Mar 3, 2020

View reviewed changes

added explanation for base64 string length

b9b42e1

layomia approved these changes Mar 4, 2020

View reviewed changes

layomia assigned alanisaac Mar 4, 2020

layomia merged commit cbd5166 into dotnet:master Mar 4, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional code coverage in System.Text.Json #33019

Additional code coverage in System.Text.Json #33019

alanisaac commented Feb 29, 2020 •

edited

Loading

ahsonkhan left a comment

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

ahsonkhan Mar 2, 2020

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

ahsonkhan Mar 2, 2020

alanisaac Mar 3, 2020

alanisaac commented Mar 3, 2020

layomia left a comment

	if (pooledArray != null)
	{
	byteSpan.Clear();
	ArrayPool<byte>.Shared.Return(pooledArray);
	}

	yield return new object[] { GenerateRandomInvalidLargeString(true) };
	yield return new object[] { GenerateRandomInvalidLargeString(includeEscapedCharacter: true) };

	for (int i = 1; i < charArray.Length; i++)
	for (int i = 1; i < charArray.Length - 1; i++)

	JsonException ex = Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Employee>(json, s_deserializerOptionsPreserve));
	Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<Employee>(json, s_deserializerOptionsPreserve));

	[InlineData(10, 0)]
	[InlineData(10, 0)] // 0 (or default) max depth is treated as 64.

Additional code coverage in System.Text.Json #33019

Additional code coverage in System.Text.Json #33019

Conversation

alanisaac commented Feb 29, 2020 • edited Loading

ahsonkhan left a comment

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

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

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

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

alanisaac commented Mar 3, 2020

layomia left a comment

Choose a reason for hiding this comment

alanisaac commented Feb 29, 2020 •

edited

Loading