-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: Added a set of tests for the DBParamGroup Name #3446
Conversation
@radeksimko I added a small func that creates a RandomString of fixed length. This may be useful for other tests. If you think that should be moved to a different location then let me know |
if len(errors) != 1 { | ||
t.Fatalf("Expected the DB Parameter Group Name to trigger a validation 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.
Lots of tests! Since they're all testing the same method with different inputs, can we re-write this to be a table test?
func TestResourceAWSDBParameterGroupName_validation(t *testing.T) {
cases := []struct {
Value string
ErrCount int
}{
{
Value: "tEsting123",
ErrCount: 1,
},
{
Value: "testing123!",
ErrCount: 1,
},
# [.. so on ..]
}
for _, tc := range cases {
_, errors := validateDbParamGroupName(tc.Value, "SampleKey")
if len(errors) != ErrCount {
t.Fatalf("Expected the DB Parameter Group Name to trigger a validation 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.
ooohhh! I didn't realise this. Updating right now
Thanks @catsby
Small nitpicks but otherwise 👍 |
…inputs as per feedback
Thanks @catsby! This is now taken care of :) (This is a really useful Go tip for me!)
|
@catsby this build is working on my local machine but yet fails on travis. Any reason why? |
…is an edge case that this could actually trigger a failure due to not allowing to start with a number
ok false alarm - this was due to an edgecase in my tests where the randomString generator could have given me a string starting with a number and thus failed the test |
Glad you figured it out, I was confused as well since it was passing for me. Either way, I assume you're patching? Thanks! |
@catsby all patched and ready for merge :) |
Thanks! |
provider/aws: Added a set of tests for the DBParamGroup Name
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
@radeksimko as requested in #3279