-
Notifications
You must be signed in to change notification settings - Fork 8
add basic testDelete for Subaccount #146
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
just put a comment to create a follow up issue to fix the error we found here.
Looks like working on the tests here was paid out already ;)
}, | ||
}, | ||
want: want{ | ||
err: errors.New("Deletion Pending: Current status: Deleting"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tripped over that one during our call.
It actually looks like there is an implementation error which returns an error here always in the first reconcile run. In the next one it will resolve itself. I have actually seen this error occasionally in my cluster, but thought its an API hickup since it resolved itself.
So yes your testing here is great, but can you create a follow up issue to fix the implementation and maybe put a comment here that this is actually wrong and requires fixing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok sure will do so, created an issue here: #155, was also wondering whats the reason to return error
@@ -6,6 +6,7 @@ import ( | |||
|
|||
// ContainsError While testing there is no point in mimicking wrapped error hierarchies, but we do want to distinguish check | |||
// whether an error is part of the stacktrace | |||
// when error message itself contains :, the split contains logic is not enough, add compare of error message itself |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes actually I think with newer go versions some error.Is function has been introduced that makes this manually check pointless, but thats good enough for now as well.
Relates to: #83
Adds Testing for Subaccount controller.