-
Notifications
You must be signed in to change notification settings - Fork 796
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
[keyvault] fix test recording sanitizers #22911
base: main
Are you sure you want to change the base?
Conversation
require.NotNil(t, roleDefinition.Type) | ||
|
||
if *roleDefinition.ID == *updatedDefinition.ID { | ||
updatedDefinitionCount++ |
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.
🤔 it would be nice to cover the pager in playback and you could do that if ID
weren't sanitized, or you didn't assert on updatedDefinitionCount
in playback. Either works but I'd probably disable the sanitizer because this ID isn't secret, then double check that there isn't some other ID which is secret.
@@ -48,7 +45,13 @@ func TestRoleDefinition(t *testing.T) { | |||
// test create definition | |||
createdDefinition, err := client.CreateOrUpdateRoleDefinition(context.Background(), scope, name, parameters, nil) | |||
require.NoError(t, err) | |||
require.Equal(t, name, *createdDefinition.Name) | |||
|
|||
if recording.GetRecordMode() == recording.PlaybackMode { |
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 suggest either removing the name
sanitizer (should be this one, names in these tests aren't secret) or asserting only in recording & live modes
#22869
Fixing tests to be compatible in playback mode with the new test sanitizers