[GitHub] [avro] BarryDahlberg commented on a change in pull request #1070: AVRO-2633 - csharp include schema doc

[GitBox <gi...@apache.org>]

BarryDahlberg commented on a change in pull request #1070:
URL: https://github.com/apache/avro/pull/1070#discussion_r577943118



##########
File path: lang/csharp/src/apache/test/Schema/SchemaTests.cs
##########
@@ -69,8 +69,12 @@ public class SchemaTests
             typeof(SchemaParseException), Description = "No fields")]
         [TestCase("{\"type\":\"record\",\"name\":\"LongList\", \"fields\": \"hi\"}",
             typeof(SchemaParseException), Description = "Fields not an array")]
-        [TestCase("[{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns1\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}," + 
+        [TestCase("[{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns1\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}," +
                    "{\"type\": \"record\",\"name\": \"Test\",\"namespace\":\"ns2\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}]")]
+
+        // Doc
+        [TestCase("{\"type\": \"record\",\"name\": \"Test\",\"doc\": \"Test Doc\",\"fields\": [{\"name\": \"f\",\"type\": \"long\"}]}")]
+

Review comment:
       There are a few doc tests in this file already like `TestRecordDoc` and `TestEnumDoc`. These check that the field is parsed in without error, which is all that this case does as well. None of them actually check that the doc string is written when serialising though.
   
   Changing these existing tests to do something like write the schema out and parse it again will give you test cases for the whole cycle which I think is quite important to this PR, something like:
   
   ```
           public void TestRecordDoc(string s, string expectedDoc)
           {
               var parsed = Schema.Parse(s) as RecordSchema;
   
               Assert.IsNotNull(parsed);
               Assert.AreEqual(expectedDoc, parsed.Documentation);
   
               var roundTrip = Schema.Parse(parsed.ToString()) as RecordSchema;
   
               Assert.IsNotNull(roundTrip);
               Assert.AreEqual(expectedDoc, roundTrip.Documentation);
           }
   ```
   
   I'm not clear on how empty strings should be treated though.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org