Add protobuf conformance tests v1 #2404

Dogacel · 2023-08-07T18:06:12Z

Includes the message type used to run conformance tests by Google.

My intention is to catch and demonstrate some serialization errors & missing features. Commented parts are proven to be non-working. Probably some tests can be improved by using generators and multiple iterations with edge cases.

A part of

…vides

Dogacel · 2023-08-12T19:44:49Z

Hi team, would be glad if some review can be done, I am hoping those bugs will be resolved and protobuf module can hopefully promote to stable after all tests are fixed 🙏

I am willing to help whenever I am free.

shanshin · 2023-08-14T08:38:56Z

Hi,
sorry for the delay and thanks for the PR!
We will review it in the next few days.

pdvrieze · 2023-08-14T10:03:55Z

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/conformance/Proto3OneofTest.kt

+            if (restored.hasOneofString()) assertEquals(message.oneofString, restored.oneofString)
+            if (restored.hasOneofBytes()) assertContentEquals(message.oneofBytes, restored.oneofBytes.toByteArray())
+            if (restored.hasOneofBool()) assertEquals(message.oneofBool, restored.oneofBool)
+            if (restored.hasOneofUint64()) assertEquals(message.oneofUint64, restored.oneofUint64.toULong())
+            if (restored.hasOneofFloat()) assertEquals(message.oneofFloat, restored.oneofFloat)
+            if (restored.hasOneofDouble()) assertEquals(message.oneofDouble, restored.oneofDouble)
+            if (restored.hasOneofEnum()) assertEquals(message.oneofEnum?.name, restored.oneofEnum?.name)


Rather than checking restored, you should probably check message

sandwwraith · 2023-08-14T15:37:08Z

Just for the clarification of your intentions — do you want this PR to be merged with commented code? Or you want to leave it as a showcase until all issues are fixed?

Dogacel · 2023-08-15T09:05:17Z

Just for the clarification of your intentions — do you want this PR to be merged with commented code? Or you want to leave it as a showcase until all issues are fixed?

Hi,

My personal preference is to merge those tests, either with comments or mark them as @Ignored (which causes some duplicate code that's why I choose to comment). My intention to add them to tests was to showcase what's missing in the code and also make testing easier for developers.

As library maintainers, please let me know how you want to merge this. If you are against keeping comments, I think it is fine to remove them and open issues in GitHub to track. But I think we should merge the non-commented tests anyway because conformance tests are an important indicator to verify functionality.

sandwwraith · 2023-08-17T13:39:55Z

I see.

But I think we should merge the non-commented tests anyway because conformance tests are an important indicator to verify functionality.

Yes, this is a great point. I suggest to do the following:

Carefully split test data to working and non-working parts (latter in a separate file). It looks to me that amount of duplicated code won't be that big. Non-working parts should be @Ignored, not commented.
Create an issue(s) (if there's not one already) that describes missing parts. Issue number should be added as a comment to @Ignore annotation. You can create one big issue or several separate, whatever suits you best.

Does that sound plausible to you?

Dogacel · 2023-08-20T16:52:31Z

I see.

But I think we should merge the non-commented tests anyway because conformance tests are an important indicator to verify functionality.

Yes, this is a great point. I suggest to do the following:

Carefully split test data to working and non-working parts (latter in a separate file). It looks to me that amount of duplicated code won't be that big. Non-working parts should be @Ignored, not commented.

Create an issue(s) (if there's not one already) that describes missing parts. Issue number should be added as a comment to @Ignore annotation. You can create one big issue or several separate, whatever suits you best.

Does that sound plausible to you?

Hi,

Yep that sounds plausible. I am currently traveling and also sick so will complete this when I have time and feel better 😆

sandwwraith · 2023-08-21T12:33:59Z

No problems, take your time and get well!

Dogacel · 2023-08-23T20:54:18Z

@sandwwraith Updated the tests and opened three issues:

Protobuf - uintXX packed array can't be serialized / deserialized #2418
Protobuf - sintXX, fixedXX and sfixedXX array can't be serialized / deserialized #2419
Protobuf - sintXX, fixedXX and sfixedXX map keys and values not serialized / deserialized correctly. #2417

I believe they all are related.

pdvrieze

Overall it looks ok to me (not an official dev). I'd say split the individual tests off into separate functions per type. Otherwise I find the code containing a lot of repetitive elements that don't help to make the code clear. Given that the types are local to the tests the API itself isn't really an issue, but perhaps the resulting verbosity isn't ideal (I don't really know how to solve the clarity issue though without adding just as much complexity in other places).

pdvrieze · 2023-09-02T14:00:33Z

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/conformance/Proto3OneofTest.kt

+    fun default() {
+        listOf(
+            KTestMessageProto3Oneof(oneofUint32 = 150u),
+            KTestMessageProto3Oneof(oneofNestedMessage = KTestMessagesProto3Message.KNestedMessage(a = 150)),
+            KTestMessageProto3Oneof(oneofString = "150"),
+            KTestMessageProto3Oneof(oneofBytes = "150".toByteArray()),
+            KTestMessageProto3Oneof(oneofBool = true),
+            KTestMessageProto3Oneof(oneofUint64 = 150uL),
+            KTestMessageProto3Oneof(oneofFloat = 150f),
+            KTestMessageProto3Oneof(oneofDouble = 150.0),
+            KTestMessageProto3Oneof(oneofEnum = KTestMessagesProto3Enum.KNestedEnum.BAR),
+        ).forEach { message ->


Rather than testing all types in a single function it would make sense to have the inner loop in a separate function and just have separate test functions for the different data members. This would help with testing individual types, and brings more visibility (each actual test function would just call the actual implementation with the message as parameter). It would also allow testing each type even in case of some failure.

I agree with that. This would allow to determine what exact instance of KTestMessageProto3Oneof caused failure.

Or, If you prefer a different way, you can add an assertion message to each assertion, but this looks much more boilerplate-ish.

I tried to reduce boilerplate by creating some verification functions. Let me know how it looks now.

pdvrieze · 2023-09-02T14:19:37Z

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/conformance/Proto3PrimitiveTest.kt

+        assertEquals(message.optionalInt32, restored.optionalInt32)
+        assertEquals(message.optionalInt64, restored.optionalInt64)
+        assertEquals(message.optionalUint32, restored.optionalUint32.toUInt())
+        assertEquals(message.optionalUint64, restored.optionalUint64.toULong())
+        assertEquals(message.optionalSint32, restored.optionalSint32)
+        assertEquals(message.optionalSint64, restored.optionalSint64)
+        assertEquals(message.optionalFixed32, restored.optionalFixed32)
+        assertEquals(message.optionalFixed64, restored.optionalFixed64)
+        assertEquals(message.optionalSfixed32, restored.optionalSfixed32)
+        assertEquals(message.optionalSfixed64, restored.optionalSfixed64)
+        assertEquals(message.optionalFloat, restored.optionalFloat)
+        assertEquals(message.optionalDouble, restored.optionalDouble)
+        assertEquals(message.optionalBool, restored.optionalBool)
+        assertEquals(message.optionalString, restored.optionalString)


As you are already comparing the full message below these comparisons seem double.

I should have compared the fields with the serialized one rather than the restored.

Sometimes message can be serialized and deserialized it might be the same but the serialization format might be different so if deserialized by another library it can be something else. This is most obvious with signed ints (i.e. it appears as 2*int - 1)

My intention was to catch this serialized format check.

pdvrieze · 2023-09-02T14:20:30Z

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/conformance/Proto3PrimitiveTest.kt

+        assertContentEquals(message.optionalBytes, restored.optionalBytes.toByteArray())
+
+        val restoredMessage = ProtoBuf.decodeFromByteArray<KTestMessagesProto3Primitive>(restored.toByteArray())
+        assertEquals(message, restoredMessage.copy(optionalBytes = message.optionalBytes))


You may add a comment on what you are doing here (compensating for the fact that you didn't provide an equals implementation for the message type).

Yes, a comment would be nice

Yep, added where I do this trick.

sandwwraith

Looks great! Just fix the minor issues I've mentioned, and we can merge this.

sandwwraith · 2023-09-13T14:57:19Z

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/conformance/Proto3PrimitiveTest.kt

+        assertContentEquals(message.optionalBytes, restored.optionalBytes.toByteArray())
+
+        val restoredMessage = ProtoBuf.decodeFromByteArray<KTestMessagesProto3Primitive>(restored.toByteArray())
+        assertEquals(message, restoredMessage.copy(optionalBytes = message.optionalBytes))


Yes, a comment would be nice

sandwwraith · 2023-09-13T15:04:25Z

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/conformance/Proto3OneofTest.kt

+    fun default() {
+        listOf(
+            KTestMessageProto3Oneof(oneofUint32 = 150u),
+            KTestMessageProto3Oneof(oneofNestedMessage = KTestMessagesProto3Message.KNestedMessage(a = 150)),
+            KTestMessageProto3Oneof(oneofString = "150"),
+            KTestMessageProto3Oneof(oneofBytes = "150".toByteArray()),
+            KTestMessageProto3Oneof(oneofBool = true),
+            KTestMessageProto3Oneof(oneofUint64 = 150uL),
+            KTestMessageProto3Oneof(oneofFloat = 150f),
+            KTestMessageProto3Oneof(oneofDouble = 150.0),
+            KTestMessageProto3Oneof(oneofEnum = KTestMessagesProto3Enum.KNestedEnum.BAR),
+        ).forEach { message ->


I agree with that. This would allow to determine what exact instance of KTestMessageProto3Oneof caused failure.

Or, If you prefer a different way, you can add an assertion message to each assertion, but this looks much more boilerplate-ish.

Dogacel · 2023-09-23T12:10:08Z

Looks great! Just fix the minor issues I've mentioned, and we can merge this.

Thanks. It took me a bit of time to catch-up but fixed some stuff and ready for another round of review. Thanks!

sandwwraith

Amazing job, thank you very much!

add conformance tests using test_messages_proto3 construct google pro…

16b56c1

…vides

Dogacel changed the base branch from master to dev August 7, 2023 18:06

Dogacel mentioned this pull request Aug 7, 2023

Protobuf repeated uint, fixed and enum serialization not working #2401

Closed

qwwdfsad requested a review from shanshin August 7, 2023 21:04

Dogacel mentioned this pull request Aug 12, 2023

Maps with fixed keys can't be serialized Dogacel/kotlinx-protobuf-gen#9

Open

pdvrieze reviewed Aug 14, 2023

View reviewed changes

Dogacel added 2 commits August 23, 2023 15:15

Merge branch 'dev' into dogac/add-protobuf-conformance-tests-v1

a33e69c

update tests and descriptions

8a89a96

Dogacel requested a review from pdvrieze September 1, 2023 15:58

pdvrieze reviewed Sep 2, 2023

View reviewed changes

sandwwraith reviewed Sep 13, 2023

View reviewed changes

refactor tests slightly

85e8f67

Dogacel requested review from sandwwraith and pdvrieze September 23, 2023 12:10

sandwwraith approved these changes Oct 10, 2023

View reviewed changes

sandwwraith merged commit a675cb3 into Kotlin:dev Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add protobuf conformance tests v1 #2404

Add protobuf conformance tests v1 #2404

Dogacel commented Aug 7, 2023 •

edited

Loading

Dogacel commented Aug 12, 2023 •

edited

Loading

shanshin commented Aug 14, 2023

pdvrieze Aug 14, 2023

sandwwraith commented Aug 14, 2023

Dogacel commented Aug 15, 2023

sandwwraith commented Aug 17, 2023

Dogacel commented Aug 20, 2023

sandwwraith commented Aug 21, 2023

Dogacel commented Aug 23, 2023

pdvrieze left a comment

pdvrieze Sep 2, 2023

sandwwraith Sep 13, 2023

Dogacel Sep 23, 2023

pdvrieze Sep 2, 2023

Dogacel Sep 5, 2023

pdvrieze Sep 2, 2023

sandwwraith Sep 13, 2023

Dogacel Sep 23, 2023

sandwwraith left a comment

sandwwraith Sep 13, 2023

sandwwraith Sep 13, 2023

Dogacel commented Sep 23, 2023

sandwwraith left a comment

Add protobuf conformance tests v1 #2404

Add protobuf conformance tests v1 #2404

Conversation

Dogacel commented Aug 7, 2023 • edited Loading

Dogacel commented Aug 12, 2023 • edited Loading

shanshin commented Aug 14, 2023

Choose a reason for hiding this comment

sandwwraith commented Aug 14, 2023

Dogacel commented Aug 15, 2023

sandwwraith commented Aug 17, 2023

Dogacel commented Aug 20, 2023

sandwwraith commented Aug 21, 2023

Dogacel commented Aug 23, 2023

pdvrieze 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

sandwwraith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dogacel commented Sep 23, 2023

sandwwraith left a comment

Choose a reason for hiding this comment

Dogacel commented Aug 7, 2023 •

edited

Loading

Dogacel commented Aug 12, 2023 •

edited

Loading