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
    



---