Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

integration Junit for OSS #778

Merged
merged 17 commits into from
Feb 19, 2024
Merged

integration Junit for OSS #778

merged 17 commits into from
Feb 19, 2024

Conversation

vikasgupta78
Copy link
Collaborator

integration Junit for OSS

sonalgoyal
sonalgoyal requested changes Feb 17, 2024
View reviewed changes
common/common-test/src/test/java/zingg/common/core/executor/TrainerTester.java Outdated
}

@Override
public void validateResults() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should validate that a model has been created - you can add a separate issue for that and we can take it up later

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#783 created

spark/spark-test/src/test/java/zingg/spark/core/executor/TestSparkExecutors.java Show resolved Hide resolved
common/common-test/src/test/java/zingg/common/core/executor/MatcherTester.java Outdated


// returns df1.intersect(df2)
public abstract ZFrame<D, R, C> intersect(ZFrame<D, R, C> df1, ZFrame<D, R, C> df2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should define these test methods in Zframe itself so that we dont have to go down to spark/snowflake specific stuff in the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in commit 2c7f744

spark/spark-test/src/test/java/zingg/spark/core/executor/TestSparkExecutors.java Outdated

protected SparkTrainingDataFinder getTrainingDataFinder() throws ZinggClientException {
SparkTrainingDataFinder stdf = new SparkTrainingDataFinder(ctx);
stdf.init(args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move init to executortester.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the benefit in that? Moreover if something special needs to be done for an instance let it be done by concrete implementation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That specific thing will anyways be done in the derived class. Setting up the test in one places ensures no code repetition and that all derived classes are executing the same tests, unless explicitly overridden

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in commit 6423793

spark/spark-test/src/test/java/zingg/spark/core/executor/TestSparkExecutors.java Outdated
}

@Test
public void testExecutors() throws ZinggClientException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this to the base class with S,D,R,C,T. Only getTrainingDataFinder etc should be overwritten to return concrete class here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in commit e18399e

common/common-test/pom.xml Outdated
<packaging>jar</packaging>
<dependencies>
<dependency>
<groupId>zingg</groupId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move tests to common test as discussed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done as part of commits 3a79946 , 2184a84 and 6b97885

Please let me know if spark-test module also needs to be removed and tests moved to spark-core

@vikasgupta78 vikasgupta78 mentioned this pull request Feb 18, 2024
Open
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 18, 2024
View reviewed changes
common/common-test/src/test/java/zingg/common/core/executor/LabellerTester.java Fixed Show fixed Hide fixed
common/common-test/src/test/java/zingg/common/core/executor/MatcherTester.java Fixed Show fixed Hide fixed
common/common-test/src/test/java/zingg/common/core/executor/MatcherTester.java Fixed Show fixed Hide fixed
common/common-test/src/test/java/zingg/common/core/executor/MatcherTester.java Fixed Show fixed Hide fixed
common/common-test/src/test/java/zingg/common/core/executor/MatcherTester.java Fixed Show fixed Hide fixed
common/common-test/src/test/java/zingg/common/core/executor/MatcherTester.java Fixed Show fixed Hide fixed
common/common-test/src/test/java/zingg/common/core/executor/TrainingDataFinderTester.java Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 18, 2024
View reviewed changes
common/core/src/test/java/zingg/common/core/executor/LabellerTester.java
assertTrue(matchCount > 1);
long unmatchCount = dfMarked.filter(notMatchCond).count();
assertTrue(unmatchCount > 1);
LOG.info("matchCount : "+ matchCount + ", unmatchCount : " + unmatchCount);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
common/core/src/test/java/zingg/common/core/executor/MatcherTester.java
double score1 = tpCount*1.0d/(tpCount+fpCount);
double score2 = tpCount*1.0d/(tpCount+fnCount);

LOG.info("False negative " + fnCount);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
common/core/src/test/java/zingg/common/core/executor/MatcherTester.java
double score2 = tpCount*1.0d/(tpCount+fnCount);

LOG.info("False negative " + fnCount);
LOG.info("True positive " + tpCount);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
common/core/src/test/java/zingg/common/core/executor/MatcherTester.java

LOG.info("False negative " + fnCount);
LOG.info("True positive " + tpCount);
LOG.info("False positive " + fpCount);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
common/core/src/test/java/zingg/common/core/executor/MatcherTester.java
LOG.info("False negative " + fnCount);
LOG.info("True positive " + tpCount);
LOG.info("False positive " + fpCount);
LOG.info("precision " + score1);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
common/core/src/test/java/zingg/common/core/executor/MatcherTester.java
LOG.info("True positive " + tpCount);
LOG.info("False positive " + fpCount);
LOG.info("precision " + score1);
LOG.info("recall " + tpCount + " denom " + (tpCount+fnCount) + " overall " + score2);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
common/core/src/test/java/zingg/common/core/executor/TrainingDataFinderTester.java

long trainingDataCount = df.count();
assertTrue(trainingDataCount > 10);
LOG.info("trainingDataCount : "+ trainingDataCount);

Check warning

Code scanning / PMD

Logger calls should be surrounded by log level guards. Warning

Logger calls should be surrounded by log level guards.
@sonalgoyal sonalgoyal merged commit 3ab5482 into zinggAI:enterprise Feb 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonalgoyal sonalgoyal sonalgoyal requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vikasgupta78 @sonalgoyal