GH-3032: RecoveryCallback on RetryingDeserializer #3035
Conversation
| * @param recoveryCallback {@link RecoveryCallback} to execute | ||
| * @since 3.1.2 | ||
| */ | ||
| public void setRecoveryCallback(@Nullable RecoveryCallback<T> recoveryCallback) { |
There was a problem hiding this comment.
Setters must follow just after constructors.
| @Test | ||
| void retryDeserializerWithRecoveryCallback() throws InterruptedException { | ||
| NonRecoverableDeser delegate = new NonRecoverableDeser(); | ||
| RetryingDeserializer<String> rdes = new RetryingDeserializer<>(delegate, new RetryTemplate()); |
There was a problem hiding this comment.
You also can just inject a lambda which would throw that exception.
I don't think it matters how many time we retry.
Only what is important for this test that RecoveryCallback is called.
| NonRecoverableDeser delegate = new NonRecoverableDeser(); | ||
| RetryingDeserializer<String> rdes = new RetryingDeserializer<>(delegate, new RetryTemplate()); | ||
| rdes.setRecoveryCallback(context -> { | ||
| delegate.latch.countDown(); |
There was a problem hiding this comment.
You can just sent here a result of mock() and then verify in the end that it was called.
There is no any race conditions: everything in the test is called sequentially.
| } | ||
|
|
||
| @Override | ||
| public void close() { |
There was a problem hiding this comment.
The Deserializer is functional interface, so no need to override those default methods.
See my comment above about its simple lambda for the RetryingDeserializer ctor.
Aren't we going to backport the change to |
| RetryingDeserializer<String> rdes = new RetryingDeserializer<>((s, b) -> { | ||
| throw new RuntimeException(); | ||
| }, new RetryTemplate()); | ||
| RecoveryCallback<String> recoveryCallback = mock(RecoveryCallback.class); |
There was a problem hiding this comment.
I know that is going to be nit-picking, but I'll shoot it anyway.
The Mockito starting with some version does not need the type argument.
Just RecoveryCallback<String> recoveryCallback = mock(); is enough to make it working.
And the code looks nicer.
| RecoveryCallback<String> recoveryCallback = mock(RecoveryCallback.class); | ||
| rdes.setRecoveryCallback(recoveryCallback); | ||
| rdes.deserialize("my-topic", "my-data".getBytes()); | ||
| verify(recoveryCallback, times(1)).recover(any(RetryContext.class)); |
There was a problem hiding this comment.
The times(1) is there by default:
public static <T> T verify(T mock) {
return MOCKITO_CORE.verify(mock, times(1));
}
Why do you use this explicitly?
| rdes.setRecoveryCallback(recoveryCallback); | ||
| rdes.deserialize("my-topic", "my-data".getBytes()); | ||
| verify(recoveryCallback, times(1)).recover(any(RetryContext.class)); | ||
| rdes.close(); |
There was a problem hiding this comment.
There is nothing to close. See our delegate lambda.
IMHO the test should be as short as possible and be focused on the specific task.
If tests are too complicated and they are flaky, it is hard to find the reason and solution.
I know: this one is not, but better to have a good habit for less code - better support!
There was a problem hiding this comment.
ok, will cleanup, i was just following the original test patterns.
|
@artembilan How can I find when Should I assume in the next spring boot release? Is there a link with planned future spring boot releases? Thanks again! |
|
yes, this one is included in released yesterday Spring Boot |
Fixes: #3032