[ZEPPELIN-6033] Add tests for de/serialization #4790

JooHyukKim · 2024-08-13T04:10:37Z

What is this PR for?

What type of PR is it?

Improvement

Todos

- Write tests

What is the Jira issue?

This PR adds test coverage to existing classes that are part of de/serialization.
Note that this PR is pure in sense that Gson vs Jackson is not covered here, but only data processing part.

How should this be tested?

Strongly recommended: add automated unit tests for any new or changed behavior
Outline any manual steps to test the PR here.

Screenshots (if appropriate)

Questions:

Does the license files need to update?
Is there breaking changes for older versions?
Does this needs documentation?

JooHyukKim · 2024-08-13T07:26:41Z

Closing (will re-open after fix)

The tests are failling. Seems like I didn't commit everything upstream.

seung-00 · 2024-08-19T16:22:06Z

...-interpreter/src/test/java/org/apache/zeppelin/cluster/event/ClusterMessageSerDeserTest.java

+
+    private static String a2q(String json) {
+        return json.replace("'", "\"");
+    }


Is a2q really necessary? If not, I think using a pure JSON format without a2q could make the test clearer. Keeping the test focused on the target, which in this case is JSON serialization/deserialization, could help keep the test simple.
Also, in this PR HeliumRegistrySerializerTest does not use a2q.

Sure I will remove a2q methods.
I disagree with you and a2q methods don't hinder us from focusing on the SUT.
I just don't have much preference over a2q here.

seung-00 · 2024-08-19T16:25:22Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumRegistrySerializerTest.java

+            "}";
+
+    @Test
+    void testDeserialization() throws IOException {


How about unifying the method names?
toJson/fromJson in RemoteZeppelinServerResourceTest vs. deserialization/serialization in HeliumRegistrySerializerTest.

seung-00 · 2024-08-19T16:43:55Z

...erpreter/src/test/java/org/apache/zeppelin/interpreter/RemoteZeppelinServerResourceTest.java

+    }
+
+    @Test
+    void fromJson_withLargeDataSet() {


It seems like a performance test. In my opinion, it's better to avoid performance testing in unit tests. I think unit tests and performance tests have different purposes.

Oh my bad. I think the test name made you get the wrong idea 😅.
The test is intended to test (sort of) long data as input.
There is a whole lot of Zeppelin integration tests that run wayyyy over a minute 😆
This test won't even leave a dent on the performance of Zeppelin test suite.

I changed the name from LargeDataSet to LongDataset.

@JooHyukKim
Does using long data actually change the functionality here? If it's just a unit test, the toJson test in this class already seems to cover the same function.

seung-00 · 2024-08-19T17:18:33Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumRegistrySerializerTest.java

+
+class HeliumRegistrySerializerTest {
+
+    private String HELIUM_PACKAGE_JSON_1 = "{\n" +


Why is there a "1" at the end of the variable? 👀

I figured there could be more?

@JooHyukKim
If the '1' doesn't have a specific meaning, how about removing it?

seung-00 · 2024-08-19T17:21:58Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumRegistrySerializerTest.java

+        assertNotNull(registry.getAll());
+        assertEquals(1, registry.getAll().size());
+
+        HeliumPackage heliumPackage = registry.getAll().stream().findFirst().orElseThrow(RuntimeException::new);


It seems like an action, so wouldn't it be more natural to move it to the "When" section?

This is not an action at all, just another value assertion following...

assertEquals(1, registry.getAll().size());

But I do think it could be simplified.

seung-00 · 2024-08-19T17:27:44Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumRegistrySerializerTest.java

+        // When
+        // Define the output file
+        Path dir = Files.createDirectory(Paths.get("./" + UUID.randomUUID().toString()));
+        File newFile = Files.createTempFile(dir, UUID.randomUUID().toString(), ".json").toFile();


It looks like temporary files are created with each test run. It would be better to add a cleanup step.

Clean up is already implemented.
There is a delete- hook at line 56.
What I think we need is moving the delete-hook line up to right below this line.
WDYT, @seung-00 ?

@JooHyukKim
You're right. But for me, the temporary directory (path: /zeppelin-zengine) hasn't been deleted. Could you check please?

Strange, I added delete-on-exit hook on the directory too, just in case.

seung-00 · 2024-08-19T17:39:49Z

...erpreter/src/test/java/org/apache/zeppelin/interpreter/RemoteZeppelinServerResourceTest.java

+            String key, String value,
+            String key2, String value2
+    ) {
+        // TreeMap to ensure serialization order


The ClusterMessageSerDeserTest uses a HashMap. Are there no issues like this?

Hmmmm🤔 , I don't think so.
Or do you have anything in mind?
TreeMap is actually used to make test results deterministic.

seung-00 · 2024-08-19T17:44:08Z

...-interpreter/src/test/java/org/apache/zeppelin/cluster/event/ClusterMessageSerDeserTest.java

+
+import static org.junit.jupiter.api.Assertions.*;
+
+class ClusterMessageSerDeserTest {


The name SerDeser seems a bit implicit to me. In my opinion, a clearer name, even if longer, would be better, such as ClusterMessageSerializationTest.

Makes sense 👍🏼Will change.

seung-00 · 2024-08-23T14:43:54Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/helium/HeliumRegistrySerializerTest.java

+
+    }
+
+    private File createHeliumPackage(Path testDir, String JSON) throws IOException {


This method isn't used. If it's not necessary, how about removing it?

JooHyukKim · 2024-08-23T15:38:10Z

Thank you @seung-00 for review!

JooHyukKim · 2024-10-20T13:55:24Z

Closing due to inactivity and to give room for other contributions.

It feels like unifying de/serialization library won't be done efficiently with everything (other works) going on right now.

...

7888a50

JooHyukKim closed this Aug 13, 2024

JooHyukKim added 2 commits August 19, 2024 11:48

...

6158fec

Update HeliumRegistrySerializerTest.java

27b30be

JooHyukKim reopened this Aug 19, 2024

seung-00 reviewed Aug 19, 2024

View reviewed changes

seung-00 reviewed Aug 23, 2024

View reviewed changes

JooHyukKim added 6 commits August 24, 2024 00:16

Remove unusued method

7209d76

Remove a2q

72a0866

Simplify value fetching element on HeliumPackage ser test

7715579

Add delete on shutdown hook to directory too

8781512

Make ClusterMessageSerDeserTest name more explicit

ee44f20

Clairfy test name

8966521

JooHyukKim requested a review from seung-00 August 23, 2024 15:38

JooHyukKim closed this Oct 20, 2024

JooHyukKim deleted the ZEPPELIN-6033 branch October 20, 2024 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-6033] Add tests for de/serialization #4790

[ZEPPELIN-6033] Add tests for de/serialization #4790

JooHyukKim commented Aug 13, 2024

JooHyukKim commented Aug 13, 2024

seung-00 Aug 19, 2024

JooHyukKim Aug 23, 2024

seung-00 Aug 19, 2024

seung-00 Aug 19, 2024

JooHyukKim Aug 23, 2024

seung-00 Aug 23, 2024

seung-00 Aug 19, 2024

JooHyukKim Aug 23, 2024

seung-00 Aug 23, 2024

seung-00 Aug 19, 2024

JooHyukKim Aug 23, 2024

seung-00 Aug 19, 2024

JooHyukKim Aug 23, 2024

seung-00 Aug 23, 2024 •

edited

Loading

JooHyukKim Aug 23, 2024 •

edited

Loading

seung-00 Aug 19, 2024 •

edited

Loading

JooHyukKim Aug 23, 2024 •

edited

Loading

seung-00 Aug 19, 2024 •

edited

Loading

JooHyukKim Aug 23, 2024

seung-00 Aug 23, 2024

JooHyukKim Aug 23, 2024

JooHyukKim commented Aug 23, 2024

JooHyukKim commented Oct 20, 2024 •

edited

Loading


		class HeliumRegistrySerializerTest {

		private String HELIUM_PACKAGE_JSON_1 = "{\n" +


		import static org.junit.jupiter.api.Assertions.*;

		class ClusterMessageSerDeserTest {


		}

		private File createHeliumPackage(Path testDir, String JSON) throws IOException {

[ZEPPELIN-6033] Add tests for de/serialization #4790

[ZEPPELIN-6033] Add tests for de/serialization #4790

Conversation

JooHyukKim commented Aug 13, 2024

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

JooHyukKim commented Aug 13, 2024

Closing (will re-open after fix)

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

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

seung-00 Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

JooHyukKim Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

seung-00 Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

JooHyukKim Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

seung-00 Aug 19, 2024 • edited Loading

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

JooHyukKim commented Aug 23, 2024

JooHyukKim commented Oct 20, 2024 • edited Loading

seung-00 Aug 23, 2024 •

edited

Loading

JooHyukKim Aug 23, 2024 •

edited

Loading

seung-00 Aug 19, 2024 •

edited

Loading

JooHyukKim Aug 23, 2024 •

edited

Loading

seung-00 Aug 19, 2024 •

edited

Loading

JooHyukKim commented Oct 20, 2024 •

edited

Loading