Add test coverage for serialization/deserialization for upcoming refactoring#1806
Add test coverage for serialization/deserialization for upcoming refactoring#1806jeandersonbc merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for various Nexo model classes and a resource-loading utility to verify serialization and deserialization. The feedback suggests optimizing performance by using a static Gson instance and enhancing test maintainability by implementing equals and hashCode methods for more comprehensive round-trip validation.
| @Test | ||
| public void testShouldSerializeAndDeserializeFromMockFile() throws IOException { | ||
| String mockJson = NexoTestUtils.readResource("mocks/terminal-api/abort-request.json"); | ||
| Gson terminalApiGson = TerminalAPIGsonBuilder.create(); |
There was a problem hiding this comment.
The Gson object is created within the test method. Since Gson instances are thread-safe and their creation can be expensive, it's a good practice to create it only once per class. Consider making terminalApiGson a private static final field in the AbortRequestTest class. This improves performance and code clarity.
| assertEquals(deserializedAbortRequest.getAbortReason(), roundTripAbortRequest.getAbortReason()); | ||
| assertEquals( | ||
| deserializedAbortRequest.getMessageReference().getServiceID(), | ||
| roundTripAbortRequest.getMessageReference().getServiceID()); | ||
| assertEquals( | ||
| deserializedAbortRequest.getMessageReference().getDeviceID(), | ||
| roundTripAbortRequest.getMessageReference().getDeviceID()); | ||
| assertEquals( | ||
| deserializedAbortRequest.getMessageReference().getSaleID(), | ||
| roundTripAbortRequest.getMessageReference().getSaleID()); | ||
| assertEquals( | ||
| deserializedAbortRequest.getMessageReference().getPOIID(), | ||
| roundTripAbortRequest.getMessageReference().getPOIID()); | ||
| assertEquals( | ||
| deserializedAbortRequest.getDisplayOutput().getInfoQualify(), | ||
| roundTripAbortRequest.getDisplayOutput().getInfoQualify()); |
There was a problem hiding this comment.
The round-trip validation is done by asserting a subset of the object's properties. This is brittle and incomplete. For example, MessageReference.MessageCategory is asserted during the initial deserialization but not in this round-trip check.
A more robust and maintainable approach would be to implement equals() and hashCode() methods on the AbortRequest class and its nested objects. This would allow you to replace these multiple assertions with a single, comprehensive check:
assertEquals(deserializedAbortRequest, roundTripAbortRequest);This ensures that all fields are compared, making the test more thorough and easier to maintain when new fields are added to the model.
| assertEquals(deserializedAbortRequest.getAbortReason(), roundTripAbortRequest.getAbortReason()); | |
| assertEquals( | |
| deserializedAbortRequest.getMessageReference().getServiceID(), | |
| roundTripAbortRequest.getMessageReference().getServiceID()); | |
| assertEquals( | |
| deserializedAbortRequest.getMessageReference().getDeviceID(), | |
| roundTripAbortRequest.getMessageReference().getDeviceID()); | |
| assertEquals( | |
| deserializedAbortRequest.getMessageReference().getSaleID(), | |
| roundTripAbortRequest.getMessageReference().getSaleID()); | |
| assertEquals( | |
| deserializedAbortRequest.getMessageReference().getPOIID(), | |
| roundTripAbortRequest.getMessageReference().getPOIID()); | |
| assertEquals( | |
| deserializedAbortRequest.getDisplayOutput().getInfoQualify(), | |
| roundTripAbortRequest.getDisplayOutput().getInfoQualify()); | |
| assertEquals(deserializedAbortRequest, roundTripAbortRequest); |
| @Test | ||
| public void testShouldSerializeAndDeserializeFromMockFile() throws IOException { | ||
| String mockJson = NexoTestUtils.readResource("mocks/terminal-api/display-request.json"); | ||
| Gson terminalApiGson = TerminalAPIGsonBuilder.create(); |
There was a problem hiding this comment.
The Gson object is created within the test method. Since Gson instances are thread-safe and their creation can be expensive, it's a good practice to create it only once per class. Consider making terminalApiGson a private static final field in the DisplayRequestTest class. This improves performance and code clarity.
| assertEquals( | ||
| "Please present card", | ||
| roundTripDisplayRequest | ||
| .getDisplayOutput() | ||
| .get(0) | ||
| .getOutputContent() | ||
| .getOutputText() | ||
| .get(0) | ||
| .getText()); |
There was a problem hiding this comment.
This round-trip validation only checks a single, deeply nested property. This is very incomplete and provides little value. The initial deserialization checks more properties (e.g., Device, InfoQualify).
A more robust and maintainable approach would be to implement equals() and hashCode() methods on the DisplayRequest class and its nested objects. This would allow you to replace this assertion with a single, comprehensive check:
assertEquals(deserializedDisplayRequest, roundTripDisplayRequest);This ensures that all fields are compared, making the test more thorough and easier to maintain.
assertEquals(deserializedDisplayRequest, roundTripDisplayRequest);| @Test | ||
| public void testShouldSerializeAndDeserializeFromMockFile() throws IOException { | ||
| String mockJson = NexoTestUtils.readResource("mocks/terminal-api/login-request.json"); | ||
| Gson terminalApiGson = TerminalAPIGsonBuilder.create(); |
There was a problem hiding this comment.
The Gson object is created within the test method. Since Gson instances are thread-safe and their creation can be expensive, it's a good practice to create it only once per class. Consider making terminalApiGson a private static final field in the LoginRequestTest class. This improves performance and code clarity.
| assertEquals( | ||
| deserializedLoginRequest.getDateTime().toString(), | ||
| roundTripLoginRequest.getDateTime().toString()); | ||
| assertEquals( | ||
| deserializedLoginRequest.getTokenRequestedType(), | ||
| roundTripLoginRequest.getTokenRequestedType()); | ||
| assertEquals( | ||
| deserializedLoginRequest.getCustomerOrderReq(), | ||
| roundTripLoginRequest.getCustomerOrderReq()); | ||
| assertEquals( | ||
| deserializedLoginRequest.getPOISerialNumber(), roundTripLoginRequest.getPOISerialNumber()); |
There was a problem hiding this comment.
The round-trip validation is done by asserting a subset of the object's properties. This is brittle and incomplete. Many properties asserted during the initial deserialization (e.g., SaleSoftware details, SaleTerminalData) are not checked here.
A more robust and maintainable approach would be to implement equals() and hashCode() methods on the LoginRequest class and its nested objects. This would allow you to replace these multiple assertions with a single, comprehensive check:
assertEquals(deserializedLoginRequest, roundTripLoginRequest);This ensures that all fields are compared, making the test more thorough and easier to maintain.
assertEquals(deserializedLoginRequest, roundTripLoginRequest);| @Test | ||
| public void testShouldSerializeAndDeserializeFromMockFile() throws IOException { | ||
| String mockJson = NexoTestUtils.readResource("mocks/terminal-api/message-header.json"); | ||
| Gson terminalApiGson = TerminalAPIGsonBuilder.create(); |
There was a problem hiding this comment.
The Gson object is created within the test method. Since Gson instances are thread-safe and their creation can be expensive, it's a good practice to create it only once per class. Consider making terminalApiGson a private static final field in the MessageHeaderTest class. This improves performance and code clarity.
| assertEquals( | ||
| deserializedMessageHeader.getProtocolVersion(), | ||
| roundTripMessageHeader.getProtocolVersion()); | ||
| assertEquals( | ||
| deserializedMessageHeader.getMessageClass(), roundTripMessageHeader.getMessageClass()); | ||
| assertEquals( | ||
| deserializedMessageHeader.getMessageCategory(), | ||
| roundTripMessageHeader.getMessageCategory()); | ||
| assertEquals( | ||
| deserializedMessageHeader.getMessageType(), roundTripMessageHeader.getMessageType()); | ||
| assertEquals(deserializedMessageHeader.getServiceID(), roundTripMessageHeader.getServiceID()); | ||
| assertEquals(deserializedMessageHeader.getDeviceID(), roundTripMessageHeader.getDeviceID()); | ||
| assertEquals(deserializedMessageHeader.getSaleID(), roundTripMessageHeader.getSaleID()); | ||
| assertEquals(deserializedMessageHeader.getPOIID(), roundTripMessageHeader.getPOIID()); |
There was a problem hiding this comment.
While these assertions cover all properties of MessageHeader, the test can be made more concise and maintainable.
By implementing equals() and hashCode() on the MessageHeader class, you can replace all these individual assertions with a single line:
assertEquals(deserializedMessageHeader, roundTripMessageHeader);This approach is cleaner and scales better if new fields are added to MessageHeader in the future.
assertEquals(deserializedMessageHeader, roundTripMessageHeader);| @Test | ||
| public void testShouldSerializeAndDeserializeFromMockFile() throws IOException { | ||
| String mockJson = NexoTestUtils.readResource("mocks/terminal-api/message-reference.json"); | ||
| Gson terminalApiGson = TerminalAPIGsonBuilder.create(); |
There was a problem hiding this comment.
The Gson object is created within the test method. Since Gson instances are thread-safe and their creation can be expensive, it's a good practice to create it only once per class. Consider making terminalApiGson a private static final field in the MessageReferenceTest class. This improves performance and code clarity.
| assertEquals( | ||
| deserializedMessageReference.getMessageCategory(), | ||
| roundTripMessageReference.getMessageCategory()); | ||
| assertEquals( | ||
| deserializedMessageReference.getServiceID(), roundTripMessageReference.getServiceID()); | ||
| assertEquals( | ||
| deserializedMessageReference.getDeviceID(), roundTripMessageReference.getDeviceID()); | ||
| assertEquals(deserializedMessageReference.getSaleID(), roundTripMessageReference.getSaleID()); | ||
| assertEquals(deserializedMessageReference.getPOIID(), roundTripMessageReference.getPOIID()); |
There was a problem hiding this comment.
While these assertions cover all properties of MessageReference, the test can be made more concise and maintainable.
By implementing equals() and hashCode() on the MessageReference class, you can replace all these individual assertions with a single line:
assertEquals(deserializedMessageReference, roundTripMessageReference);This approach is cleaner and scales better if new fields are added to MessageReference in the future.
assertEquals(deserializedMessageReference, roundTripMessageReference);
gcatanese
left a comment
There was a problem hiding this comment.
I think the Gemini comments are not critical, we can go ahead
Description
In the scope of JAXB migration, we need some sanity check to verify that the changes will work.
This is not a full complete set, but I sampled a few classes to add coverage.
It would be helpful to get some feedback about the tests and whether we could improve something for the upcoming work.
Tested scenarios