KAFKA-14133: Migrate StateDirectory mock in TaskManagerTest to Mockito

[TaklaGerguis]

This pull requests migrates the StateDirectory mock in TaskManagerTest from EasyMock to Mockito.
The change is restricted to a single mock to minimise the scope and make it easier for review.

The reasoning as to why we would like to migrate a single mock rather than all mocks in the file at the same time has been discussed in #12607 (comment)

It takes the same approach as in:
#13529
#13621
#13681
#13711
et al.

I had to change the names of some of the test methods because of a problem similar to the one discussed in #13711 (comment)

[TaklaGerguis]

This was reported by Mockito as an unnecessary stubbing. I checked the code and it appears we never call this as part of the test. Let me know in case I am wrong.

[cadonna]

That is because there is a bug in the test.

Line

makeTaskFolders(taskId00.toString(), task01.toString());

should be

makeTaskFolders(taskId00.toString(), taskId01.toString());

Variable task01 holds a StateMachineTask and not a TaskId. Consequently, task01.toString() returns the string representation of a StateMachineTask and not of a TaskId which leads to a TaskIdFormatException in this call and skips the lock.

Please fix the bug and readd the stub.

[cadonna]

@TaklaGerguis Thanks for the PR!

Sorry for the delay!

Here my feedback.

[cadonna]

I am afraid that this does not make too much sense. But do not be sad, this is not the first time I see this and it is mainly because EasyMock is not explicit enough on when a stub/expectation is specified.
Under EasyMock the call to stateDirectory.unlock(task) specifies an expectation on mock stateDirectory, because the call is done before replay(stateDirectory) is called. With Mockito, stateDirectory.unlock(task) is just a call on the mock stateDirectory as the method under test would do. So in this code you call a method on the mock and then you immediately verify whether the call was done.
This code should rather be:

    private void expectUnlockFor(final TaskId... tasks) {
        for (final TaskId task : tasks) {
            Mockito.verify(stateDirectory, Mockito.atLeastOnce()).unlock(task);
        }
    }

and you need to mnove the call to expectUnlockFor() to after the call to the method under test.

[cadonna]

Please see my comment on expectUnlockFor(). You should replace this with expectUnlockFor(taskId02).

[cadonna]

I think you can remove this line as you did a couple of lines above since Mockito returns an empty list by default.

[cadonna]

That is because there is a bug in the test.

Line

makeTaskFolders(taskId00.toString(), task01.toString());

should be

makeTaskFolders(taskId00.toString(), taskId01.toString());

Variable task01 holds a StateMachineTask and not a TaskId. Consequently, task01.toString() returns the string representation of a StateMachineTask and not of a TaskId which leads to a TaskIdFormatException in this call and skips the lock.

Please fix the bug and readd the stub.

[github-actions]

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

[github-actions]

This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.