[FLINK-6617][table] Improve JAVA and SCALA logical plans consistent test #3943

sunjincheng121 · 2017-05-19T09:43:52Z

In this PR. I had make some changes as follows:

Add all the tableAPI JAVA and SCALA logical plans consistent test.
Add validation folder for steam which keep consistent with batch.
Refactoring the test structure.

General
- The pull request references the related JIRA issue ("[FLINK-6617][table] Improve JAVA and SCALA logical plans consistent test")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
Tests & Build
- Functionality added by the pull request is covered by tests
- mvn clean verify has been executed successfully locally or a Travis build has passed

twalthr

Thanks for the PR @sunjincheng121. The changes look very good, I had only minor comments. The validation packages should only contain validation tests and not the regular tests. Would be great if you could split the tests.

stringexpr for Java == Scala
validation for testing wrong table programs
runtime for ITCases
remaining packages for unit tests

As a summary, right?

twalthr · 2017-05-23T08:24:16Z

...a/org/apache/flink/table/api/scala/stream/table/validation/GroupWindowAggregationsTest.scala

 */

-package org.apache.flink.table.api.scala.stream.table
+package org.apache.flink.table.api.scala.stream.table.validation


I think tests in validation packages should all test for a ValidationException. Can you split this class?

twalthr · 2017-05-23T08:26:39Z

flink-libraries/flink-table/src/test/scala/org/apache/flink/table/utils/TableTestBase.scala

 case class BatchTableTestUtil() extends TableTestUtil {
-
+  val javaEnv = mock(classOf[JExecutionEnvironment])
+  val javaTableEnv = TableEnvironment.getTableEnvironment(javaEnv)


Maybe we can rename the Scala environment to tableEnv to be consistent.

twalthr · 2017-05-23T08:27:35Z

...ala/org/apache/flink/table/api/scala/stream/table/stringexpr/UnionStringExpressionTest.scala

+import org.junit.Test
+
+class UnionStringExpressionTest extends TableTestBase {
+  @Test


Add an empty line here.

sunjincheng121 · 2017-05-23T16:08:02Z

Hi @twalthr Thanks a lot for your reviewing. I completely agree with your suggestions. I petty want refactoring the test structure. I have updated the PR. according your comments.

Thanks,
SunJincheng

sunjincheng121 · 2017-05-31T10:08:31Z

I once again seriously consider the Refactoring work, according to the following principles updated the PR.

First, the test type and purpose(4 types):

Unit Test: to ensure the correctness of the most granular features
StringExpression Test: Verify the consistency of java and scalaAPI
Validation Test: Check the API exception information
Integration Test: Integration testing, verifying runtime and end-to-end correctness

   Second, the directory structure(folders) and purpose:

The TEST directory contains all the paths (folders) of the SRC directory - the functional tests included three type test case (Unit Test, StringExpression Test,Validation Test)
   Note: runtime directory contains both runtime (Unit Test, StringExpression Test,Validation Test), and All table API/SQL Integration Test.

Third, the file name naming convention:

Unit Test -> functionName + Test
StringExpression Test -> functionName + StringExpressionTest
Validation Test -> functionName + ValidationTest
Integration Test -> FunctionName + ITCase

After the change of the directory structure, as shown below:

@twalthr @fhueske @wuchong The change is big, so I need your help to ensure that the direction of change is correct.

Best,
SunJincheng

wuchong · 2017-05-31T11:33:35Z

Hi @sunjincheng121 , thanks for your great work. I like your proposal. The test structure refactoring looks good to me. I think it's good to put all the IT cases in one place.

sunjincheng121 · 2017-06-01T05:01:19Z

Hi @wuchong Thanks for your approval.
In addition to refactoring the test structure, I think that in addition to ITCast other tests should be unified use of batchTestUtil and streamTestUtil. What do you think? @wuchong @twalthr @fhueske

sunjincheng121 · 2017-06-09T10:33:12Z

Rebase code...

sunjincheng121 · 2017-06-21T15:28:56Z

May be I need split this PR into a couple of small PRs. What do you think? or have any other suggestions @fhueske @wuchong @twalthr

fhueske · 2017-06-21T20:30:23Z

I think that is a very good idea. A review of a +6k -4k LOC PR takes a lot of time and has a good chance to be interrupted by other issues. Several smaller PRs would be much easier to review and merge.
It would be great if you could do that @sunjincheng121.

Thank you very much!

twalthr · 2017-06-26T08:16:43Z

@sunjincheng121 the structure looks very good. I would volunteer to go over the changes as a whole and merge this. If @fhueske @wuchong are fine with this? Can you rebase the PR a last time?

wuchong · 2017-06-26T08:28:39Z

Thanks @twalthr , I'm fine with this ,a great +1!

fhueske · 2017-06-26T09:03:42Z

@twalthr, sure! Thanks for taking care of this PR.

sunjincheng121 · 2017-06-26T09:31:03Z

@twalthr Extremely grateful If you can review and merge this PR after I rebase the PR.

sunjincheng121 · 2017-06-27T09:12:29Z

@twalthr I have rebase the PR. I appreciated if you can review it. And i'll quick updated it when you left comments.

Thanks,
SunJincheng

sunjincheng121 · 2017-06-27T14:25:51Z

Update again. Because the new merged code.

twalthr · 2017-06-30T13:13:31Z

Thanks for the update @sunjincheng121. I will go over all changes and try to merge this.

sunjincheng121 · 2017-07-04T03:45:31Z

Thanks @twalthr ! I have rebased the code and updated the PR.

Thanks,
SunJincheng

sunjincheng121 · 2017-07-06T01:16:55Z

Rebase code according master.

twalthr · 2017-07-11T13:23:29Z

@wuchong @sunjincheng121 @fhueske

I tried to simplify the package structure a little bit. I based my changes on top of this PRs commit.

My goals:

splitting a class like (ExpressionReductionTest) in 6 classes is not very helpful
tests are named like the operator or feature that they are testing e.g. Calc, Distinct etc. (we don't need CastingITCase, CalcITCase, ScalarFunctionITCase; CalcITCase covers everything)
all tests have the same name everywhere (OverWindowTest, OverWindowValidationTest, OverWindowITCase, OverWindowStringExpressionTest)
no packages with one class
org.apache.flink.table.api contains all tests that test the translation from batch/stream/table/sql APIs
org.apache.flink.table.plan contains all tests that modify the plan
org.apache.flink.table.runtime contains all ITCases and runtime relevant tests
all other package contain unit tests for the classes of the same package

You can find my branch here: master...twalthr:FLINK-6617

fhueske · 2017-07-11T13:42:28Z

Hi @twalthr and @sunjincheng121, I looked over the package structure and naming of the tests and this looks good to me.

+1 to merge from my side.

sunjincheng121 · 2017-07-11T15:35:09Z

Hi @twalthr @fhueske thanks a lot for your review.
@twalthr your improve changes are make the structure of the package is clearer which make sense to me.

+1 to merge.

twalthr · 2017-07-13T07:49:58Z

Merging...

twalthr requested changes May 23, 2017

View reviewed changes

sunjincheng121 force-pushed the FLINK-6617-PR branch from fb56921 to 79aa4b2 Compare May 23, 2017 15:59

sunjincheng121 force-pushed the FLINK-6617-PR branch 3 times, most recently from 2028b87 to 93d9e6d Compare May 29, 2017 23:40

sunjincheng121 force-pushed the FLINK-6617-PR branch 3 times, most recently from a69c0b2 to 897034c Compare May 31, 2017 09:41

sunjincheng121 force-pushed the FLINK-6617-PR branch from 897034c to f326b3d Compare May 31, 2017 10:27

sunjincheng121 force-pushed the FLINK-6617-PR branch 2 times, most recently from f4ea048 to 30093e3 Compare June 1, 2017 04:51

sunjincheng121 force-pushed the FLINK-6617-PR branch 4 times, most recently from fc4ba3b to c09594a Compare June 4, 2017 23:50

sunjincheng121 force-pushed the FLINK-6617-PR branch from c09594a to 0947427 Compare June 9, 2017 10:32

sunjincheng121 force-pushed the FLINK-6617-PR branch from 0947427 to b1b70bf Compare June 27, 2017 00:38

sunjincheng121 force-pushed the FLINK-6617-PR branch from b1b70bf to 8ee49df Compare June 27, 2017 14:24

sunjincheng121 force-pushed the FLINK-6617-PR branch from 8ee49df to c698f3b Compare July 4, 2017 03:40

[FLINK-6617][table] Improve JAVA and SCALA logical plans consistent test

de0f189

sunjincheng121 force-pushed the FLINK-6617-PR branch from c698f3b to de0f189 Compare July 6, 2017 01:13

asfgit closed this in ecde7bc Jul 13, 2017

rmetzger added the component=API/TableSQL label Mar 14, 2019

flinkbot added component=TableSQL/API and removed component=API/TableSQL labels Mar 17, 2022

[FLINK-6617][table] Improve JAVA and SCALA logical plans consistent test #3943

[FLINK-6617][table] Improve JAVA and SCALA logical plans consistent test #3943

Uh oh!

Conversation

sunjincheng121 commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

twalthr May 23, 2017

Choose a reason for hiding this comment

Uh oh!

twalthr May 23, 2017

Choose a reason for hiding this comment

Uh oh!

twalthr May 23, 2017

Choose a reason for hiding this comment

Uh oh!

sunjincheng121 commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunjincheng121 commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wuchong commented May 31, 2017

Uh oh!

sunjincheng121 commented Jun 1, 2017

Uh oh!

sunjincheng121 commented Jun 9, 2017

Uh oh!

sunjincheng121 commented Jun 21, 2017

Uh oh!

fhueske commented Jun 21, 2017

Uh oh!

twalthr commented Jun 26, 2017

Uh oh!

wuchong commented Jun 26, 2017

Uh oh!

fhueske commented Jun 26, 2017

Uh oh!

sunjincheng121 commented Jun 26, 2017

Uh oh!

sunjincheng121 commented Jun 27, 2017

Uh oh!

sunjincheng121 commented Jun 27, 2017

Uh oh!

twalthr commented Jun 30, 2017

Uh oh!

sunjincheng121 commented Jul 4, 2017

Uh oh!

sunjincheng121 commented Jul 6, 2017

Uh oh!

twalthr commented Jul 11, 2017

Uh oh!

fhueske commented Jul 11, 2017

Uh oh!

sunjincheng121 commented Jul 11, 2017

Uh oh!

twalthr commented Jul 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sunjincheng121 commented May 19, 2017 •

edited

Loading

sunjincheng121 commented May 23, 2017 •

edited

Loading

sunjincheng121 commented May 31, 2017 •

edited

Loading