Refactor the testIsTaskCurrent test case to improve the test experience

[Codegass]

Description & Motivation

The test case testIsTaskCurrent 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:

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 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 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 and taskFromStorage value creation into the testIsTaskCurrentTaskFromStorage unit test case with the mock setup(line 4021-4023) and isTaskCurrent assertion functions(line 4036) for the first testing scenario.
   • Extract line 3887 to line 3888, modifiedDataSchema value creation, and taskFromStorageMismatchedDataSchema value creation into the testIsTaskCurrentTaskFromStorageMismatchedDataSchema unit test case with the mock setup(line 4024-4026) and isTaskCurrent assertion functions(line 4037) for the second testing scenario.
   • Extract line 3887 to line 3888, modifiedTuningConfig value creation, and taskFromStorageMismatchedTuningConfig value creation into the testIsTaskCurrentTaskFromStorageMismatchedTuningConfig unit test case with the mock setup(line 4027-4029) and isTaskCurrent assertion functions(line 4038) for the third testing scenario.
   • Extract line 3887 to line 3888 and taskFromStorageMismatchedPartitionsWithTaskGroup value creation into the testIsTaskCurrentTaskFromStorageMismatchedPartitionsWithTaskGroup unit test case with the mock setup(line 4030-4032) and isTaskCurrent assertion functions(line 4038) 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.)

[abhishekagarwal87]

@Codegass - changes sound good to me. It will be great if you can create a PR.

[github-actions]

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.

[github-actions]

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.