test: Remove redundant assertions for defaults in S3 unit tests #240
Conversation
s3/tests/unit.tftest.hcl
Outdated
| @@ -1,7 +1,7 @@ | |||
| variables { | |||
| vpc_name = "s3-test-vpc-name" | |||
| application = "s3-test-application" | |||
| environment = "s3-test-environment" | |||
| environment = "s3-test-environmnent" | |||
There was a problem hiding this comment.
Why have you introduced the misspelling?
s3/main.tf
Outdated
| enable_key_rotation = false | ||
| is_enabled = true | ||
| key_usage = "ENCRYPT_DECRYPT" | ||
| customer_master_key_spec = "SYMMETRIC_DEFAULT" |
There was a problem hiding this comment.
These are all just the default values anyway. Do we need them to be explicit?
There was a problem hiding this comment.
They were coming out as null in the tests.
Maybe that's a symptom of running the tests on a plan?
There was a problem hiding this comment.
If that is the output from the plan, it's probably better to expect null rather than setting a bunch of variables when we don't need to or it looks like we've set them deliberately.
There was a problem hiding this comment.
I definitely don't want to explicitly check for null when that is not the actual expected result.
Should we even be testing for the default values on a plan if they are just coming back as null?
There was a problem hiding this comment.
Perhaps not. We should obviously test things we're explicitly setting. I think setting things to the default values to make tests pass is wrong, and it will lead to people wondering why we set them to those values in the future.
WillGibson
changed the title
I guess the question is, were the tests right, or was the code right?