-
Notifications
You must be signed in to change notification settings - Fork 48
Conversation
The token is not really used for the NodeController, so it's only for consistency.
val, err := ExtractToken(secret, "key") | ||
assert.Error(t, err) | ||
assert.Empty(t, val) | ||
// this case should ideally fail with "missing token X" error |
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.
you can add message arg to assert
function call. These comments can be moved there, imo.
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.
Hmm, the comment would only appear when the check fails which don't think it's the intention here, right? This comment is more of a TODO.
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.
So this test is actually useless. Either change the validation logic in ExtractToken
to check for empty strings or remove it.
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'll just remove the comment. I personally find an empty string a valid response for non-specific Secret field getter, and the usual receiver, dynatrace_client.NewClient() already validates for empty strings anyway.
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.
Please squash the commits into one before merging - as they are all minor changes.
Some small changes:
ExtractToken
(formerlyGetToken
) tests tooneagent-utils
package.istio_integration_test.go
file name.