Skip to content

Conversation

@mjsax
Copy link
Member

@mjsax mjsax commented Jun 25, 2015

migrated test from execute() to collect()
subtask of https://issues.apache.org/jira/browse/FLINK-2032

@mjsax
Copy link
Member Author

mjsax commented Jun 25, 2015

I adapted all tests except for DataSinkITCase and ExecutionEnvironmentITCase.

  • DataSinkITCase -> seems to test writing to file explicit; would not make sense to change it (tell me, if I am wrong)
  • ExecutionEnvironmentITCase ->uses LocalCollectionOutputFormat, and is not writing to disc already

@StephanEwen
Copy link
Contributor

All in all this looks good.

The only think that strikes me as odd is the test pattern (which was used also before the refactoring). I am not a big fan of validating results in an @After method. I find the tests more readable if they simply call the comparison themselves at the end:

List<Tuple2<String, Integer>> result = data.collect()

String expected = "Hello,2\n" + 
                  "Dude,5\n" 

compareResultAsTuples(expexted, result)

This also solves the issue with the raw type lists, which is usually an indicator that something is not modeled in the right way (unless you dynamically load or instantiate types).

@StephanEwen
Copy link
Contributor

This comment is not a blocker for merging this. Let's just not write future tests like this, and whenever we touch something, we could on-the-fly migrate the tests to not follow this pattern any more...

@mjsax
Copy link
Member Author

mjsax commented Jun 26, 2015

I agree. I just did not want to change the current pattern... However, we should do it the right way in this PR, too. I will update the code accordingly.

@mjsax
Copy link
Member Author

mjsax commented Jun 27, 2015

Done. Failing tests revile a Kafka problem (in my Travis, too https://travis-ci.org/mjsax/flink/builds/68598571). Is it known? Module flink-tests passed in all runs.

Should be ready to get merged.

@rmetzger
Copy link
Contributor

@gyfora re-enabled a disabled Kafka test in this pull request: https://github.com/apache/flink/pull/747/files
The test is known to be unstable, thats why I disabled it for the time being. Can you disable it again in your PR?

@gyfora
Copy link
Contributor

gyfora commented Jun 27, 2015

Sorry about re-enabling the failing test, I completely missed it...
Matthias please @ignore it :P

Btw @rmetzger do we have a JIRA for this?

@mjsax
Copy link
Member Author

mjsax commented Jun 27, 2015

Done. If Travis is green, please merge. :)

 -> for package 'org.apache.flink.test.javaApiOperators'
deaktivated instable test (see comment section https://issues.apache.org/jira/browse/FLINK-2275)
@StephanEwen
Copy link
Contributor

Looks good, let me merge this...

StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Jul 1, 2015
 -> for package 'org.apache.flink.test.javaApiOperators'

 Seactivated unstable test (see comment section https://issues.apache.org/jira/browse/FLINK-2275)

This closes apache#866
@asfgit asfgit closed this in a137321 Jul 1, 2015
@mjsax mjsax deleted the flink-2275-migrateFlinkTests branch July 1, 2015 20:28
nikste pushed a commit to nikste/flink that referenced this pull request Sep 29, 2015
 -> for package 'org.apache.flink.test.javaApiOperators'

 Seactivated unstable test (see comment section https://issues.apache.org/jira/browse/FLINK-2275)

This closes apache#866
nltran pushed a commit to nltran/flink that referenced this pull request Jan 8, 2016
 -> for package 'org.apache.flink.test.javaApiOperators'

 Seactivated unstable test (see comment section https://issues.apache.org/jira/browse/FLINK-2275)

This closes apache#866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants