You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by bowenli86 <gi...@git.apache.org> on 2018/03/07 06:47:04 UTC
[GitHub] flink pull request #5649: [FLINK-8873] [DataStream API] [Tests] move unit te...
GitHub user bowenli86 opened a pull request:
https://github.com/apache/flink/pull/5649
[FLINK-8873] [DataStream API] [Tests] move unit tests of KeyedStream from DataStreamTest to KeyedStreamTest
## What is the purpose of the change
move unit tests of `KeyedStream` from `DataStreamTest` to `KeyedStreamTest`, in order to have a clearer separation
## Brief change log
added `KeyedStreamTest.java` and `KeyedStreamTest.scala`, and moved related unit tests to them
## Verifying this change
This change is already covered by existing tests, such as *KeyedStreamTest*.
## Does this pull request potentially affect one of the following parts:
none
## Documentation
none
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bowenli86/flink FLINK-8873
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5649.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 #5649
----
commit d4e372fbb21edc0507df461aa7f47a9168350a1a
Author: Bowen Li <bo...@...>
Date: 2018-03-05T19:52:37Z
[FLINK-8873] move unit tests of KeyedStream from DataStreamTest to KeyedStreamTest
----
---
[GitHub] flink issue #5649: [FLINK-8873] [DataStream API] [Tests] move unit tests of ...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/5649
I am rather against this change. Mainly because the tests that you moved are not the only ones that operate on KeyedStream. In fact most of the tests in `DataStreamTest` operate on keyed stream.
---
[GitHub] flink issue #5649: [FLINK-8873] [DataStream API] [Tests] move unit tests of ...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/5649
Hi @kl0u , can you pls take a look at this PR?
---
[GitHub] flink pull request #5649: [FLINK-8873] [DataStream API] [Tests] move unit te...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 closed the pull request at:
https://github.com/apache/flink/pull/5649
---
[GitHub] flink issue #5649: [FLINK-8873] [DataStream API] [Tests] move unit tests of ...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/5649
cc @kl0u
---
[GitHub] flink issue #5649: [FLINK-8873] [DataStream API] [Tests] move unit tests of ...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/5649
@bowenli86 What's your opinion? If you are ok with not merging it, could you close this PR?
---
[GitHub] flink issue #5649: [FLINK-8873] [DataStream API] [Tests] move unit tests of ...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/5649
I am not sure about the value of those changes. The keying and the `KeyedProcessFunction`s are applied onto `DataStream` plus tests in `DataStreamTest` cover checks if the resulting stream is keyed e.g. DataStreamTest.java:328. I seems then it is the right place for the tests you moved, at least for me.
Therefore I would be against introducing this change at all.
---
[GitHub] flink issue #5649: [FLINK-8873] [DataStream API] [Tests] move unit tests of ...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/5649
When I was developing KeyedProcessFunction, I initially wondered why there's no tests for KeyedStream, and researched and realized that they were actually mixed with DataStream tests.
I think having that clarity by separating those tests would be great. Well, I also agree it doesn't hurt that much to keep them as-is. If you feel strongly against it, I can close this PR
---