Test refactor #864

Nitish1814 · 2024-07-13T10:31:09Z

Refactored ZFrame, Block and StopWord test classes

updated jackson version in spark-client -> 2.12.0

…Nitish into testRefactor

…zingg-Nitish into testRefactor # Conflicts: # common/client/src/main/java/zingg/common/client/util/PojoToArrayConverter.java # common/client/src/main/java/zingg/common/client/util/StructTypeFromPojoClass.java # common/client/src/main/java/zingg/common/client/util/WithSession.java # common/core/src/test/java/zingg/common/core/block/TestBlockBase.java # common/core/src/test/java/zingg/common/core/preprocess/TestStopWordsBase.java # spark/client/src/main/java/zingg/spark/client/util/SparkDFObjectUtil.java # spark/core/src/test/java/zingg/common/core/block/TestSparkBlock.java # spark/core/src/test/java/zingg/common/core/preprocess/TestSparkStopWords.java # spark/core/src/test/java/zingg/common/core/util/SparkStopWordRemoverUtility.java

sonalgoyal

Common naming for test data. We can define model package inside the package of the class we are testing. Can you come up with good naming here?
Assertions should not be repeated. Move to common utility and use throughout.
Other changes requested at different places

common/client/pom.xml

common/client/src/test/java/zingg/common/client/TestZFrameBase.java

sonalgoyal · 2024-07-15T08:02:45Z

common/client/src/test/java/zingg/common/client/TestZFrameBase.java

+
+        ZFrame<D, R, C> zFrame = dfObjectUtil.getDFFromObjectList(sampleDataSet, Person.class);
+
+        List<C> columnList = (List<C>) Arrays.asList("recid", "surname", "postcode");


wont this give List ?

common/client/src/test/java/zingg/common/client/TestZFrameBase.java

common/client/src/test/java/zingg/common/client/model/PairPartOne.java

sonalgoyal · 2024-07-15T08:11:26Z

common/core/src/test/java/zingg/common/core/data/TestData.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class TestData {


rename to add the test class which is using this.

sonalgoyal · 2024-07-15T08:12:00Z

common/core/src/test/java/zingg/common/core/model/EventCluster.java

@@ -0,0 +1,23 @@
+package zingg.common.core.model;
+
+public class EventCluster {


rename to EventPair

use base class EventBase and exetnd in both Event and EventPair?

sonalgoyal · 2024-07-15T08:14:17Z

spark/client/src/test/java/zingg/client/TestSparkFrame.java

-	 }
+public class TestSparkFrame extends TestZFrameBase<SparkSession, Dataset<Row>, Row, Column, DataType> {
+    public static final Log LOG = LogFactory.getLog(TestSparkFrame.class);
+    public static IArguments args;


why do oyu need args here?

sonalgoyal · 2024-07-15T08:14:49Z

spark/client/src/test/java/zingg/client/TestSparkFrame.java

+    public static final Log LOG = LogFactory.getLog(TestSparkFrame.class);
+    public static IArguments args;
+    public static JavaSparkContext ctx;
+    public static SparkSession spark;


these should be part of the base class - when you add the concrete class here, they will automatically have the right strucgture.

sonalgoyal · 2024-08-09T07:17:59Z

spark/core/src/test/java/zingg/common/core/block/TestSparkBlock.java

+    }
+
+    protected static void setUpSpark() {
+        try {


we have a generic sparkbasetester which sets the session, we should use that

Sorry I dont see you using the common base class which sets up Spark session. Please move it from enterprise and use it here.

sonalgoyal

please check comments

sonalgoyal · 2024-08-09T07:19:47Z

spark/core/src/test/java/zingg/common/core/util/SparkStopWordRemoverUtility.java

+        stmtArgs.setFieldDefinition(fdList);
+        sparkStopWordsRemovers.add(new SparkStopWordsRemover(context,stmtArgs));
+
+        //add second stopWordRemover


setting up the stop word removers should be in generic base class

getStopWordRemovers() will provide different kind and number of stopword removers based on Spark or Snow implementation. IStopWordsRemoverutility interface only exposes getStopWordRemovers() method

sonalgoyal · 2024-08-12T08:20:15Z

spark/client/pom.xml

@@ -8,8 +8,8 @@
 	<artifactId>zingg-spark-client</artifactId>
 	<packaging>jar</packaging>
 	<properties>
-		<fasterxml.jackson.version>2.15.2</fasterxml.jackson.version>
-		<fasterxml.jackson.databind.version>2.15.2</fasterxml.jackson.databind.version>
+		<fasterxml.jackson.version>2.12.0</fasterxml.jackson.version>


why are we changing this?

sonalgoyal · 2024-08-12T08:21:51Z

common/client/src/test/java/zingg/common/client/data/TestData.java

+    //sample data classes to be used for testing
+    public static List<Person> createEmptySampleData() {
+
+        return new ArrayList<>();


No empty diamond operators

sonalgoyal · 2024-08-12T08:23:08Z

common/client/src/test/java/zingg/common/client/util/TestStringRedactor.java


-import java.util.Arrays;


please avoid changes to classes you are not touching

only removed unused import

sonalgoyal · 2024-08-12T08:23:44Z

common/core/src/test/java/zingg/common/core/data/EventTestData.java

+    public static List<Event> createSampleEventData() {
+
+        int row_id = 1;
+        List<Event> sample = new ArrayList<>();


please give class when you instantiate

It is a class static property for test purpose, there's no reason to make it instance dependent

sonalgoyal · 2024-08-12T08:27:09Z

spark/client/src/test/java/zingg/client/TestSparkFrame.java

-		ZFrame<Dataset<Row>,Row,Column> sf2 = sf.withColumn(newCol, newColVal);
- 		assertTrueCheckingExceptOutput(sf2, df.withColumn(newCol, functions.lit(newColVal)), "SparkFrame.withColumn(c, double) output is not as expected");
-	}
+    protected static void setUpSpark() {


we need to use the common testsparkbase or move the enterprise SparkSession weaving here which can inject the session. session for tests should not get created in multiple places

sonalgoyal · 2024-08-12T08:27:38Z

spark/client/src/test/java/zingg/client/TestSparkFrame.java

+            setUpSpark();
+        }
+
+        StructType schemaOfSample = new StructType(new StructField[]{


these should be coming from the base test class. Why are they here?

sonalgoyal · 2024-08-12T08:28:09Z

spark/client/src/test/java/zingg/client/TestSparkFrame.java

-		ZFrame<Dataset<Row>, Row, Column> filteredData = clusterData.filterNotNullCond(ColName.SOURCE_COL);
-		assertEquals(3,filteredData.count());
-   }
+    protected void assertTrueCheckingExceptOutput(ZFrame<Dataset<Row>, Row, Column> sf1, ZFrame<Dataset<Row>, Row, Column> sf2, String message) {


this should be part of the base test class so it can be used in enterprise and SnowFrame directly

sonalgoyal · 2024-08-12T08:28:56Z

spark/core/src/test/java/zingg/common/core/block/TestSparkBlock.java

+    }
+
+    protected static void setUpSpark() {
+        try {


Sorry I dont see you using the common base class which sets up Spark session. Please move it from enterprise and use it here.

sonalgoyal · 2024-08-12T08:30:02Z

spark/core/src/test/java/zingg/common/core/util/SparkStopWordRemoverUtility.java

+        fdList.add(eventFD);
+        IArguments stmtArgs = new Arguments();
+        stmtArgs.setFieldDefinition(fdList);
+        sparkStopWordsRemovers.add(new SparkStopWordsRemover(context,stmtArgs));


only this should be done here. the rest of the code is common and should move to an abstract class impl of IStopWordRemoverUtility

Nitish1814 added 30 commits July 5, 2024 12:13

renamed

ef1b7d0

refactored tests

f724e35

removed unused imports

bf99a8b

refactored junits

9db5849

replaced GenericRowWithSchema with R

5449bfc

moved schema classes to common

62317d6

renamed model classes

50800c5

moved TestZFrameBase to common-client

01eb4ea

added withSession interface

d099b62

reverted back jackson-module-scala version

32bb530

updated jackson version in spark-client -> 2.12.0

generalized block test

b6159b3

restructured packaging

b293e8d

created model classes for testing stop words

ed5403e

initial changes

23ee4f0

initial changes stop words

24ac62c

renamed TestStopWords to TestStopWordsBase

a6fa464

added interface StopWordRemoverUtility

f6d08bd

renamed files

e73bc3f

renamed newDataSet to newZFrame

91d92ff

generalized block test

3493470

restructured packaging

42e6079

created model classes for testing stop words

679f2d0

initial changes

59d2c22

initial changes stop words

47123e2

renamed TestStopWords to TestStopWordsBase

d2d3eaf

initial commit

0b3ca28

renamed newDataSet to newZFrame

ff3320f

added TestStopWordsBase

2d89607

added SparkStopWordRemoverUtility

df7bea9

added DFObjectUtil

18b6c40

Nitish1814 added 13 commits July 10, 2024 13:45

added IWithSession

ca3e1ca

added WithSession

4736039

added SparkDFObjectUtil

ea2d91b

added StructTypeFromPojoClass

aec8754

added SparkStructTypeFromPojoClass

0c19b29

added RowsFromObjectList

52aecb5

added PojoToArrayConverter

5be021f

used IWithSession in TestBlock

66e9d71

added IStopWordRemoverUtility

c0f3ee2

Merge branch 'junitRefactor2' of https://github.com/Nitish1814/zingg-…

41e3b3f

…Nitish into testRefactor

Renamed classes and import refactors

bed9924

renamed classes

ae8cfea

sonalgoyal requested changes Jul 15, 2024

View reviewed changes

sonalgoyal reviewed Aug 9, 2024

View reviewed changes

sonalgoyal requested changes Aug 9, 2024

View reviewed changes

Nitish1814 added 3 commits August 12, 2024 01:12

test changes in Block related classes

9d4cb4c

test changes in Block related classes

9089543

class renamed

72ad571

sonalgoyal requested changes Aug 12, 2024

View reviewed changes

Nitish1814 added 3 commits August 12, 2024 16:58

class renamed

5a06679

removed commented class

e58f51b

removed <> everywhere in code

b330a03

sonalgoyal merged commit 257851e into zinggAI:main Aug 14, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test refactor #864

Test refactor #864

Nitish1814 commented Jul 13, 2024

sonalgoyal left a comment

sonalgoyal Jul 15, 2024

sonalgoyal Jul 15, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Jul 15, 2024

sonalgoyal Jul 15, 2024

Nitish1814 Aug 11, 2024

sonalgoyal Jul 15, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Jul 15, 2024

sonalgoyal Aug 9, 2024

Nitish1814 Aug 11, 2024

sonalgoyal Aug 12, 2024

sonalgoyal left a comment

sonalgoyal Aug 9, 2024

Nitish1814 Aug 11, 2024

sonalgoyal Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024

sonalgoyal Aug 12, 2024

sonalgoyal Aug 12, 2024

Nitish1814 Aug 12, 2024


		ZFrame<D, R, C> zFrame = dfObjectUtil.getDFFromObjectList(sampleDataSet, Person.class);

		List<C> columnList = (List<C>) Arrays.asList("recid", "surname", "postcode");

		@@ -0,0 +1,23 @@
		package zingg.common.core.model;

		public class EventCluster {

Test refactor #864

Test refactor #864

Conversation

Nitish1814 commented Jul 13, 2024

sonalgoyal left a 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

Choose a reason for hiding this comment

sonalgoyal left a 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

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