gemini-code-assist[bot] commented on code in PR #37005:
URL: https://github.com/apache/beam/pull/37005#discussion_r2591604528
##########
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java:
##########
@@ -180,4 +183,58 @@ 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 testProtoCoderHandlesRawMessageInterfaceGracefully() {
+ try {
+ // Simulate the user’s pipeline where T resolves to Message
+ ProtoCoder<Message> coder = ProtoCoder.of(Message.class);
+ 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) {
+ assertEquals(
+ "Class
org.apache.beam.sdk.extensions.protobuf.ProtoCoderTest$1InvalidMessage lacks
both getDefaultInstance() and a no-arg constructor. Ensure it is a concrete
Protobuf-generated class.",
+ e.getMessage());
+ }
+ }
Review Comment:
The name of this test, `testProtoCoderWithAbstractClassThrowsException`, is
misleading. It currently passes because `InvalidMessage` is a non-static local
class, which lacks a no-arg constructor, triggering the `NoSuchMethodException`
path. To accurately test the behavior for an abstract class, `InvalidMessage`
should be `static`, which would cause an `InstantiationException`.
I suggest modifying the test to correctly check for the
`InstantiationException` case, which makes the test's name and its
implementation consistent and more robust.
```java
public void testProtoCoderWithAbstractClassThrowsException() {
try {
static 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);
}
}
```
##########
sdks/java/extensions/protobuf/src/test/java/org/apache/beam/sdk/extensions/protobuf/ProtoCoderTest.java:
##########
@@ -180,4 +183,58 @@ 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 testProtoCoderHandlesRawMessageInterfaceGracefully() {
+ try {
+ // Simulate the user’s pipeline where T resolves to Message
+ ProtoCoder<Message> coder = ProtoCoder.of(Message.class);
+ 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());
+ }
+ }
Review Comment:
This test, `testProtoCoderHandlesRawMessageInterfaceGracefully`, is an exact
duplicate of `testProtoCoderFailsWithRawMessageInterface` on lines 188-199.
Having duplicate tests increases maintenance effort. Please remove this
redundant test.
--
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]
