[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[gallenvara]

Github user gallenvara closed the pull request at:

https://github.com/apache/flink/pull/1349


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-159217613
  
Merging...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-159220295
  
@gallenvara Because I forgot add close message in the commit, the PR should 
be closed manually. Could you close this PR? This change is merged. Sorry for 
inconvenience.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[aljoscha]

Github user aljoscha commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-159214387
  
Looks good, please go ahead @chiwanpark :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-158914256
  
@gallenvara Thanks for update! Looks good to merge for me. If there is no 
objection until tomorrow, I'll merge this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[gallenvara]

Github user gallenvara commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-158838162
  
Hi, @chiwanpark . I have modified the code format and submitted a new 
commit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on a diff in the pull request:

https://github.com/apache/flink/pull/1349#discussion_r45545495
  
--- Diff: 
flink-staging/flink-table/src/test/java/org/apache/flink/api/java/table/test/CastingITCase.java
 ---
@@ -44,29 +41,13 @@ public CastingITCase(TestExecutionMode mode){
super(mode);
}
 
-   private String resultPath;
-   private String expected = "";
-
-   @Rule
-   public TemporaryFolder tempFolder = new TemporaryFolder();
-
-   @Before
-   public void before() throws Exception{
-   resultPath = tempFolder.newFile().toURI().toString();
-   }
-
-   @After
-   public void after() throws Exception{
-   compareResultsByLinesInMemory(expected, resultPath);
-   }
-
@Test
public void testAutoCastToString() throws Exception {
ExecutionEnvironment env = 
ExecutionEnvironment.getExecutionEnvironment();
TableEnvironment tableEnv = new TableEnvironment();
 
DataSource> input =
-   env.fromElements(new Tuple7(
+   env.fromElements(new Tuple7<>(
(byte) 1, (short) 1, 1, 1L, 
1.0f, 1.0d, "Hello"));
--- End diff --

I think merging L51 and L50 would be better. After omitting generic 
parameters, we don't need a newline for this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on a diff in the pull request:

https://github.com/apache/flink/pull/1349#discussion_r45545498
  
--- Diff: 
flink-staging/flink-table/src/test/java/org/apache/flink/api/java/table/test/CastingITCase.java
 ---
@@ -89,7 +68,7 @@ public void testNumericAutocastInArithmetic() throws 
Exception {
TableEnvironment tableEnv = new TableEnvironment();
 
DataSource> input =
-   env.fromElements(new Tuple7(
+   env.fromElements(new Tuple7<>(
(byte) 1, (short) 1, 1, 1L, 
1.0f, 1.0d, "Hello"));
--- End diff --

Same as above. Please remove newline starting this. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on a diff in the pull request:

https://github.com/apache/flink/pull/1349#discussion_r45545501
  
--- Diff: 
flink-staging/flink-table/src/test/java/org/apache/flink/api/java/table/test/ExpressionsITCase.java
 ---
@@ -98,11 +77,9 @@ public void testLogic() throws Exception {
"b && true, b && false, b || false, !b");
 
DataSet ds = tableEnv.toDataSet(result, Row.class);
-   ds.writeAsText(resultPath, FileSystem.WriteMode.OVERWRITE);
-
-   env.execute();
-
-   expected = "true,false,true,false";
+   List results =ds.collect();
--- End diff --

Please add a space after `=`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on a diff in the pull request:

https://github.com/apache/flink/pull/1349#discussion_r45545505
  
--- Diff: 
flink-staging/flink-table/src/test/java/org/apache/flink/api/java/table/test/FilterITCase.java
 ---
@@ -73,11 +54,9 @@ public void testAllRejectingFilter() throws Exception {
.filter("false");
 
DataSet ds = tableEnv.toDataSet(result, Row.class);
-   ds.writeAsText(resultPath, FileSystem.WriteMode.OVERWRITE);
-
-   env.execute();
-
-   expected = "\n";
+   List resutls = ds.collect();
--- End diff --

Typo in variable name: `resutls` -> `results`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on a diff in the pull request:

https://github.com/apache/flink/pull/1349#discussion_r45545486
  
--- Diff: 
flink-staging/flink-table/src/test/java/org/apache/flink/api/java/table/test/AsITCase.java
 ---
@@ -68,16 +49,14 @@ public void testAs() throws Exception {

tableEnv.fromDataSet(CollectionDataSets.get3TupleDataSet(env), "a, b, c");
 
DataSet ds = tableEnv.toDataSet(table, Row.class);
-   ds.writeAsText(resultPath, FileSystem.WriteMode.OVERWRITE);
-
-   env.execute();
-
-   expected = "1,1,Hi\n" + "2,2,Hello\n" + "3,2,Hello world\n" + 
"4,3,Hello world, " +
+   List results =ds.collect();
--- End diff --

Please add a space after `=`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[gallenvara]

Github user gallenvara commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-157236045
  
Hi, @chiwanpark . I have modified the code and committed a new commit. Can 
you help me with review work? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-157239452
  
Okay I'll review soon. Thanks for update.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[gallenvara]

Github user gallenvara commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-157236280
  
Hi, @chiwanpark . I have modified the code and submitted a new commit. Can 
you help me with review work?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-156902819
  
@gallenvara Yes, exactly right. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[gallenvara]

Github user gallenvara commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-156890933
  
Hi, @chiwanpark . Thanks for your review. I'm not very clear about your 
first suggestion. Do you mean that we should remove type description when to 
create a new generics? 
For example: 
`DataSource> input = env.fromElements(new 
Tuple2(1f, "Hello"))`
-->
`DataSource> input = env.fromElements(new 
Tuple2<>(1f, "Hello"))`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-156469184
  
Hi @gallenvara, I read your PR and have some comments.

First, since Flink 0.10, we dropped Java 6 support and can use omit types 
if the types can be inferred. So could you remove type description in that 
case? With removing, we can make codes more readable.

Second, to convert Scala sequence to Java list, using `JavaConverters` is 
better than `JavaConversions`. For example:

```scala
import scala.collection.JavaConverters._

~skipped~

val result = ds.collect()
val expected = "~~~"

TestBaseUtils.compareResultAsText(result.asJava, expected)
```

After importing `scala.collection.JavaConverters._`, we can convert Scala 
collection to Java collection with `asJava` method.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[chiwanpark]

Github user chiwanpark commented on the pull request:

https://github.com/apache/flink/pull/1349#issuecomment-156028176
  
I would like to shepherd this PR. Thanks for opening pull request 
@gallenvara. I'll review soon.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: [FLINK-2956] [tests] Migrate integration tests...

[gallenvara]

GitHub user gallenvara opened a pull request:

https://github.com/apache/flink/pull/1349

[FLINK-2956] [tests] Migrate integration tests for Table API.

Migrate integration tests of Table API from temp file to `collect()` as 
described in umbrella jira.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/gallenvara/flink migrateTestInTableAPI

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/1349.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1349


commit 0fa4c6b772aaa07ed5b8bb429ca1e0e3592e538c
Author: gallenvara 
Date:   2015-11-12T06:44:40Z

[FLINK-2956] [tests] Migrate integration tests for Table API.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---