You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/04/05 06:10:36 UTC
[GitHub] [druid] Codegass opened a new issue, #12398: Refactor the testDedup test case to improve the test experience
Codegass opened a new issue, #12398:
URL: https://github.com/apache/druid/issues/12398
## Description & Motivation
The test case [ testIsTaskCurrent ](https://github.com/apache/druid/tree/master/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3885) is testing the `supervisor.isTaskCurrent()` with 4 different scenarios (`taskFromStorage`, `taskFromStorageMismatchedDataSchema`,`taskFromStorageMismatchedTuningConfig`,`taskFromStorageMismatchedPartitionsWithTaskGroup`) in one single test case. In my humble opinion, it could be beneficial to separate the four scenarios into four individual test cases. Each test case will focus on one scenario.
The original test case is 156 lines, and after the refactoring, each test case will be around 30 lines.
For example, when testing the `supervisor.isTaskCurrent()` with `taskFromStorage`, the new case will only contain the `supervisor` and the `taskFromStorage` related variable/mock setup, and will have only one assert statement. Here is how the refactored test case for `taskFromStorage` scenario looks like:
```java
DateTime minMessageTime = DateTimes.nowUtc();
DateTime maxMessageTime = DateTimes.nowUtc().plus(10000);
KinesisSupervisor supervisor = getSupervisor(...);
KinesisIndexTask taskFromStorage = createKinesisIndexTask("id1", ...) // omitted some parameters to save the space
EasyMock.expect(taskStorage.getTask("id1"))
.andReturn(Optional.of(taskFromStorage))
.once();
replay();
Assert.assertTrue(supervisor.isTaskCurrent(42, "id1"));
verify();
```
Please see the detailed description of my proposed change below.
## Proposed changes
[ testIsTaskCurrent ](https://github.com/apache/druid/tree/master/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3885) case is aiming to test the `isTaskCurrent` function with Easymock. And the four scenarios are quite independent of each other, I suggest to :
1. Extract the arrangements shared across multiple scenarios into a setup function for the test case:
* The arrangement from [line 3886 to line 3911](https://github.com/apache/druid/tree/master/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3886-L3911) can be extracted as a function named as `isCurrentSupervisorSetup()`.
2. Extract the 4 test scenarios with their specific arrangements into 4 separate unit tests:
* Extract [line 3887 to line 3888](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3887-L3888) and [taskFromStorage value creation](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3952-L3966) into the `testIsTaskCurrentTaskFromStorage` unit test case with the mock setup([line 4021-4023](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4021-L4023)) and isTaskCurrent assertion functions([line 4036](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-servic
e/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4036)) for the first testing scenario.
* Extract [line 3887 to line 3888](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3887-L3888), [modifiedDataSchema value creation](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3913), and [taskFromStorageMismatchedDataSchema value creation](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3968-L3982) into the `testIsTaskCurrentTaskFromStorageMismatchedDataSchema` unit test case with the mock setup([line 4024-4026](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/ki
nesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4024-L4026)) and isTaskCurrent assertion functions([line 4037](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4037)) for the second testing scenario.
* Extract [line 3887 to line 3888](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3887-L3888), [modifiedTuningConfig value creation](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3915-L3950), and [taskFromStorageMismatchedTuningConfig value creation](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3984-L3999) into the `testIsTaskCurrentTaskFromStorageMismatchedTuningConfig` unit test case with the mock setup([line 4027-4029](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extens
ions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4027-L4029)) and isTaskCurrent assertion functions([line 4038](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4038)) for the third testing scenario.
* Extract [line 3887 to line 3888](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L3887-L3888) and [taskFromStorageMismatchedPartitionsWithTaskGroup value creation](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4001-L4019) into the `testIsTaskCurrentTaskFromStorageMismatchedPartitionsWithTaskGroup` unit test case with the mock setup([line 4030-4032](https://github.com/apache/druid/blob/b5195c5095a1088cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4030-L4032)) and isTaskCurrent assertion functions([line 4038](https://github.com/apache/druid/blob/b5195c5095a108
8cb06ed602704fce110232f109/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java#L4038)) for the fourth testing scenario.
3. Use the `isCurrentSupervisorSetup()` in the 4 test cases for common setup.
4. Replace the `EasyMock.replayAll()` and `EasyMock.verifyAll()` functions in the original case with `EasyMock.replay()` and `EasyMock.verify()`, and put them into each of 4 test ceases.
After this refactoring, all the test scenarios will belong to one unit test case and can be evaluated in one cycle of testing.
Hope this proposal can be helpful, and if possible, I would be more than happy to try the refactoring. (If not, comments are still appreciated.)
--
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: commits-unsubscribe@druid.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [I] Refactor the testIsTaskCurrent test case to improve the test experience (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on issue #12398:
URL: https://github.com/apache/druid/issues/12398#issuecomment-1851102784
This issue has been marked as stale due to 280 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If this issue is still
relevant, please simply write any comment. Even if closed, you can still revive the
issue at any time or discuss it on the dev@druid.apache.org list.
Thank you for your contributions.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on issue #12398: Refactor the testDedup test case to improve the test experience
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on issue #12398:
URL: https://github.com/apache/druid/issues/12398#issuecomment-1092807042
@Codegass - changes sound good to me. It will be great if you can create a PR.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org