1996fanrui commented on code in PR #23233:
URL: https://github.com/apache/flink/pull/23233#discussion_r1306493807
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/drivers/GroupReduceDriverTest.java:
##########
@@ -235,22 +237,16 @@ public void
testAllReduceDriverIncorrectlyAccumulatingMutable() {
Object[] res = result.getList().toArray();
Object[] expected =
DriverTestData.createReduceMutableDataGroupedResult().toArray();
- try {
- DriverTestData.compareTupleArrays(expected, res);
- Assert.fail(
- "Accumulationg mutable objects is expected to result
in incorrect values.");
- } catch (AssertionError e) {
- // expected
- }
+ DriverTestData.compareTupleArrays(expected, res);
Review Comment:
Reverting the change makes sense for this PR.
I analyzed this test in detail, and found the `AssertionError` is expected
from `DriverTestData.compareTupleArrays(expected, res);` for this test. We can
see FLINK-1005[1][2] added 2 tests in this class, they are:
`testAllReduceDriverIncorrectlyAccumulatingMutable` and
`testAllReduceDriverAccumulatingImmutable`. From the method name, it tests some
`Incorrectly` cases, so:
- `testAllReduceDriverIncorrectlyAccumulatingMutable` expects the
`AssertionError`
- `testAllReduceDriverAccumulatingImmutable` doesn't expect the
`AssertionError`
I checked out the commit of FLINK-1005, and run this test, the
`DriverTestData.compareTupleArrays(expected, res);` throw the `AssertionError`.
Why it doesn't throw exception now, in short:
- The `context.setMutableObjectMode(false);` doesn't work after FLINK-1285,
so the `testAllReduceDriverIncorrectlyAccumulatingMutable` became to
`testAllReduceDriverAccumulatingImmutable`, it caused
`DriverTestData.compareTupleArrays(expected, res);` doesn't throw exception
anymore.
- `fail()` still throw `AssertionError`, and catched it, so
`testAllReduceDriverIncorrectlyAccumulatingMutable` runs well, and no any
developers found it.
I created the FLINK-32965 to fix it, thanks a lot @Jiabao-Sun again for
discuss here!
[1] https://issues.apache.org/jira/browse/FLINK-1005
[2] https://github.com/apache/flink/pull/66
<img width="1673" alt="图片"
src="https://github.com/apache/flink/assets/38427477/f224a9bf-d678-4f49-a003-06589b861d9a";>
##########
flink-runtime/src/test/java/org/apache/flink/runtime/operators/drivers/GroupReduceDriverTest.java:
##########
@@ -235,22 +237,16 @@ public void
testAllReduceDriverIncorrectlyAccumulatingMutable() {
Object[] res = result.getList().toArray();
Object[] expected =
DriverTestData.createReduceMutableDataGroupedResult().toArray();
- try {
- DriverTestData.compareTupleArrays(expected, res);
- Assert.fail(
- "Accumulationg mutable objects is expected to result
in incorrect values.");
- } catch (AssertionError e) {
- // expected
- }
+ DriverTestData.compareTupleArrays(expected, res);
Review Comment:
Reverting the change makes sense for this PR.
I analyzed this test in detail, and found the `AssertionError` is expected
from `DriverTestData.compareTupleArrays(expected, res);` for this test. We can
see FLINK-1005[1][2] added 2 tests in this class, they are:
`testAllReduceDriverIncorrectlyAccumulatingMutable` and
`testAllReduceDriverAccumulatingImmutable`. From the method name, it tests some
`Incorrectly` cases, so:
- `testAllReduceDriverIncorrectlyAccumulatingMutable` expects the
`AssertionError`
- `testAllReduceDriverAccumulatingImmutable` doesn't expect the
`AssertionError`
I checked out the commit of FLINK-1005, and run this test, the
`DriverTestData.compareTupleArrays(expected, res);` throw the `AssertionError`.
Why it doesn't throw exception now, in short:
- The `context.setMutableObjectMode(false);` doesn't work after FLINK-1285,
so the `testAllReduceDriverIncorrectlyAccumulatingMutable` became to
`testAllReduceDriverAccumulatingImmutable`, it caused
`DriverTestData.compareTupleArrays(expected, res);` doesn't throw exception
anymore.
- `fail()` still throw `AssertionError`, and catched it, so
`testAllReduceDriverIncorrectlyAccumulatingMutable` runs well, and no any
developers found it.
I created the FLINK-32965 to fix it, thanks a lot @Jiabao-Sun again for
discuss here!
[1] https://issues.apache.org/jira/browse/FLINK-1005
[2] https://github.com/apache/flink/pull/66
<img width="1673" alt="图片"
src="https://github.com/apache/flink/assets/38427477/f224a9bf-d678-4f49-a003-06589b861d9a";>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
