gemini-code-assist[bot] commented on code in PR #37005:
URL: https://github.com/apache/beam/pull/37005#discussion_r2591671681
##########
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java:
##########
@@ -180,4 +183,47 @@ public void testSerialVersionID() {
long serialVersionID =
ObjectStreamClass.lookup(ProtoCoder.class).getSerialVersionUID();
assertEquals(-5043999806040629525L, serialVersionID);
}
+
+ @Test
+ public void testProtoCoderFailsWithRawMessageInterface() {
+ // Simulate the user’s pipeline where T resolves to Message
+ ProtoCoder<Message> coder = ProtoCoder.of(Message.class);
+ try {
+ coder.getParser(); // Triggers the new check in updated code
+ fail("Expected IllegalArgumentException for raw Message interface");
+ } catch (IllegalArgumentException e) {
+ assertEquals(
+ "ProtoCoder does not support the raw Message interface. Use a
concrete Protobuf-generated class.",
+ e.getMessage());
+ }
+ }
+
+ @Test
+ public void testProtoCoderWithGeneratedClass() throws Exception {
+ ProtoCoder<MessageA> coder = ProtoCoder.of(MessageA.class);
+ MessageA message = MessageA.newBuilder().setField1("Test").build();
+ byte[] encoded = CoderUtils.encodeToByteArray(coder, message);
+ MessageA decoded = CoderUtils.decodeFromByteArray(coder, encoded);
+ assertEquals(message, decoded);
+ assertEquals("Test", decoded.getField1());
+ }
+
+ @Test
+ public void testProtoCoderWithAbstractClassThrowsException() {
+ try {
+ abstract class InvalidMessage implements Message {
+ // No implementation needed for test
+ }
+ ProtoCoder<InvalidMessage> coder = ProtoCoder.of(InvalidMessage.class);
+ coder.getParser();
+ fail("Expected IllegalArgumentException for abstract class");
+ } catch (IllegalArgumentException e) {
+ assertTrue(
+ "Expected message about abstract class, but was: " + e.getMessage(),
+ e.getMessage().contains("is abstract or cannot be instantiated"));
+ assertTrue(
+ "Expected cause to be InstantiationException",
+ e.getCause() instanceof InstantiationException);
+ }
+ }
Review Comment:
The new tests for failure scenarios like using the raw `Message` interface
or an abstract class are valuable. However, the primary success path introduced
by this change—handling a `Message` implementation that lacks
`getDefaultInstance()` but has a public no-arg constructor—is not covered.
To ensure the fallback logic is working as intended, please consider adding
a test case for this scenario. It would also be beneficial to add a test for
the failure case where a class has neither `getDefaultInstance()` nor a no-arg
constructor to verify the corresponding new error message.
While implementing the `Message` interface for a test class can be verbose,
testing this core functionality is important for robustness.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
