[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2017-01-24 Thread aljoscha
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2572 manually merged --- 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 i

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-11-11 Thread StefanRRichter
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/2572 Had another look at the changes, I think it is a very well written test! +1 for merge. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub a

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-11-08 Thread aljoscha
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2572 @StefanRRichter I added more tests (see the commit) and deduplicated processing-time/event-time tests. PTAL (please take another look) 😃 --- If your project is set up for it, you can reply to t

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-28 Thread StefanRRichter
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/2572 Yeah, I know the pain ;-) but next times when we need to fix only half the amount of tests on a change it will quickly pay off (hopefully) --- If your project is set up for it, you can reply

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-28 Thread aljoscha
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2572 Yeah, alright, I'm just lazy. 😅 I'll whip something up. --- 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 ha

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-28 Thread aljoscha
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2572 Thanks for your review, @StefanRRichter! You found some areas where this can be improved. I'm not yet sure whether parameterisation for processing-time/event-time can be done without obscur

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-28 Thread StefanRRichter
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/2572 Yes, I was aware that some configuration/mocking also depends on the time domain. However what those calls do is essentially equivalent, just for different time domain. So I assume an adapter

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-28 Thread aljoscha
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2572 No, it's more or less that but there is more stuff that needs to be in that adapter, for example in `testOnProcessingTimeFire()`, I'm highlighting the places that need changing: ``` pub

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-28 Thread StefanRRichter
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/2572 My idea would be to introduce an adapter that abstracts advancing of time, where two concrete implementations exist that forward to advanceWatermark or on onProcessingTime. Or am I missing som

[GitHub] flink issue #2572: [FLINK-4552] Refactor WindowOperator/Trigger Tests

2016-10-17 Thread aljoscha
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/2572 R: @StefanRRichter @kl0u , this sits on top of #2570 so you can review that one first and then this one additional commit here. --- If your project is set up for it, you can reply to this email and