google / gson

A Java serialization/deserialization library to convert Java Objects into JSON and back
Apache License 2.0
23.16k stars 4.27k forks source link

Assertion is Missed to Validate testWriterCloseIsIdempotent Test for JsonWriter's Idempotency #2649

Closed Codegass closed 4 months ago

Codegass commented 4 months ago

Gson version

2.10.1

Java / Android version

JAVA 17

Used tools

Description

Hi, I'm relatively new to the project and while going through the unit tests for the JsonWriter class, I noticed something in the testWriterCloseIsIdempotent method that I wanted to discuss. This test checks if calling the close method on JsonWriter multiple times doesn't cause any issues, indicating the method is idempotent. Currently, the test doesn't explicitly verify that the output remains the same after the second close call, which I think might be an area we could improve on for clarity and completeness.

Current Implementation:

@Test
public void testWriterCloseIsIdempotent() throws IOException {
    StringWriter stringWriter = new StringWriter();
    JsonWriter writer = new JsonWriter(stringWriter);
    writer.beginArray();
    writer.endArray();
    writer.close();
    writer.close(); // Second call to close() to test idempotency
}

Issue and Solution

The test as is runs smoothly, which suggests that the close method behaves as expected. However, without explicit assertions to check the StringWriter's content after the second close, it feels like we might be missing an opportunity to confirm the behavior explicitly.

Would it be alright if we consider adding a couple of assertions to this test? I was thinking something along the lines of verifying that the StringWriter's content does not change after the second close call. This way, we could more clearly demonstrate the idempotency of the close method.

So I would like to suggest adding the Assertions to the test case as following:

@Test
public void testWriterCloseIsIdempotent() throws IOException {
    StringWriter stringWriter = new StringWriter();
    JsonWriter writer = new JsonWriter(stringWriter);
    writer.beginArray();
    writer.endArray();
    writer.close();

    // Verify output after first close
    final String expectedOutput = "[]";
    assertEquals(expectedOutput, stringWriter.toString());

    writer.close(); // Second call to close()

    // Verify output remains unchanged after second close, confirming idempotency
    assertEquals(expectedOutput, stringWriter.toString());
}

And if adding the assert is helpful, I would be more than happy to try submit a PR. Thank you for considering my suggestion!