Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error status handler improvements #742

Merged
merged 6 commits into from
Dec 8, 2021
Merged

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Dec 8, 2021

No description provided.

@csviri csviri self-assigned this Dec 8, 2021
when(configService.getResourceCloner()).thenReturn(ConfigurationService.DEFAULT_CLONER);
when(reconciler.reconcile(eq(customResource), any()))
.thenReturn(UpdateControl.updateResource(customResource));
when(configService.getResourceCloner()).thenReturn(new Cloner() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure to make easier to do when(..).thenReturn() with mockito. Since otherwise its a different instance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but then you're not really testing the actual behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be understood as a form of a mocking of that clone method / cloner. The goal here is not to test the cloner but only if it is called in case it's relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand but the problem is what if the behavior changes when the instance is not actually cloned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually not, is just what instance we move around, for some special cases.

@csviri csviri merged commit 02e9171 into main Dec 8, 2021
@csviri csviri deleted the error-status-handler-improvements branch December 8, 2021 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving ErrorStatusHandler to be able to set status on every retry (not last one)
2 participants