hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.7k stars 9.55k forks source link

`terraform test`: silently ignores when two test_assertions have the same component, ignoring result of all but one of them #30133

Open drewmullen opened 2 years ago

drewmullen commented 2 years ago

Terraform Version

using RC due to bug. I have also tested these issues with 1.0.8

$ tf -v
Terraform v1.1.0-rc1
on darwin_amd64

Terraform Configuration Files

Scenario 1: In this branch I expect the terraform test to take about 45m (the resources take a long time to destroy) but it only takes about 3m.

Scenario 2: When i that didnt work as expected i tried purposely getting tests to fail and i was unable to. I distilled a very simple PR to demonstrate what should be an easy failure

resource "test_assertions" "expect_failure" {
  component = "tags"

  check "fail" {
    description = "this test should fail"
    condition   = true == false
  }
  equal "scheme" {
    description = "should fail"
    got         = true
    want        = false 
  }
}

Debug Output

Will post upon request

Expected Behavior

Scenario 1: tests should take about 45m to complete Scenario 2: tests should fail

Actual Behavior

Scenario 1: tests completed within 5m Scenario 2: tests pass

Additional Context

Conversation started on discuss.hashicorp.com

References

alisdair commented 2 years ago

Hi @drewmullen, thanks for reporting this.

I'm unable to reproduce the failure with the test configuration you supplied. Here's what I did:

  1. mkdir -p tests/30133
  2. Copied your configuration into tests/30133/main.tf
  3. Ran terraform test

Here's the output:

$ terraform test
╷
│ Warning: The "terraform test" command is experimental
│
│ We'd like to invite adventurous module authors to write integration tests for
│ their modules using this command, but all of the behaviors of this command
│ are currently experimental and may change based on feedback.
│
│ For more information on the testing experiment, including ongoing research
│ goals and avenues for feedback, see:
│     https://www.terraform.io/docs/language/modules/testing-experiment.html
╵
─── Failed: 30133.tags.fail (this test should fail) ───────────────────────────
condition failed
─── Failed: 30133.tags.scheme (should fail) ───────────────────────────────────
wrong value
    got:  true
    want: false

───────────────────────────────────────────────────────────────────────────────

As you can see, both tests failed. Can you suggest what might be missing in this reproduction?

If you can share the output from your tests which are failing like this, that would be helpful too. I took a look at the linked PR but was unable to see any test output from the CI runs. Thanks!

drewmullen commented 2 years ago

@alisdair this is what im getting

git clone git@github.com:aws-ia/terraform-aws-label.git
cd terraform-aws-label
git checkout terraform-test
terraform test

╷
│ Warning: The "terraform test" command is experimental
│
│ We'd like to invite adventurous module authors to write integration tests for their modules using this command, but all of the
│ behaviors of this command are currently experimental and may change based on feedback.
│
│ For more information on the testing experiment, including ongoing research goals and avenues for feedback, see:
│     https://www.terraform.io/docs/language/modules/testing-experiment.html
╵
Success! All of the test assertions passed
alisdair commented 2 years ago

I see. Looking in more detail at that branch, the issue here is that you have multiple test_assertions resources with the same component argument. This argument must be a unique identifier, and it is invalid to have more than one resource using the same component.

With the following diff from your branch, terraform test fails successfully:

$ git diff
diff --git a/tests/examples_formatted_tags/test.tf b/tests/examples_formatted_tags/test.tf
index 55e9be9..6b9824a 100644
--- a/tests/examples_formatted_tags/test.tf
+++ b/tests/examples_formatted_tags/test.tf
@@ -14,7 +14,7 @@ locals {
 }

 resource "test_assertions" "matching_formatted_labels_awscc" {
-  component = "tags"
+  component = "tags_awscc"

   check "valid_tags_for_awscc" {
     description = "Validate tags formatting for AWSCC Provider usage. expected: {key=\"foo\", value=\"bar\"}"
@@ -23,7 +23,7 @@ resource "test_assertions" "matching_formatted_labels_awscc" {
 }

 resource "test_assertions" "matching_formatted_labels_aws" {
-  component = "tags"
+  component = "tags_aws"

   check "valid_tags_for_aws" {
     description = "Validate tags formatting for AWS Provider usage. expected: {foo = bar}"
@@ -31,7 +31,7 @@ resource "test_assertions" "matching_formatted_labels_aws" {
   }
 }
 resource "test_assertions" "expect_failure" {
-  component = "tags"
+  component = "tags_fail"

   check "fail" {
     description = "this test should fail"
$ terraform test
╷
│ Warning: The "terraform test" command is experimental
│
│ We'd like to invite adventurous module authors to write integration tests for
│ their modules using this command, but all of the behaviors of this command are
│ currently experimental and may change based on feedback.
│
│ For more information on the testing experiment, including ongoing research goals
│ and avenues for feedback, see:
│     https://www.terraform.io/docs/language/modules/testing-experiment.html
╵
─── Failed: examples_formatted_tags.tags_fail.fail (this test should fail) ─────────
condition failed
─── Failed: examples_formatted_tags.tags_fail.scheme (should fail) ─────────────────
wrong value
    got:  true
    want: false

────────────────────────────────────────────────────────────────────────────────────

With regards to your original concern: Terraform is creating and destroying the VPC resources, and you can verify this by reading the debug logs with TF_LOG=trace. I'm not sure why you're expecting them to take 45 minutes to destroy, as that hasn't been my experience in the past.

I don't think there's a bug in Terraform here, so I'm going to close this issue. But if I've misunderstood something please do let me know!

drewmullen commented 2 years ago

hey, nice! thanks for pointing that out, @alisdair ! however i dont think this is quite closed yet.

from multiple-component perspective, perhaps its not a bug but from a user experience you should either:

regarding the time outs (45m vs 3m) - please look a little closer above, theres actually 2 different repos. the one youre referring to (that i called "scenario 2") is only vpcs. but theres also this branch (which i called scenario 1) which doesnt have multiple components and is expected to take a full 45m but only takes 3m

in hindsight this should have been 2 separate issues but at the time of opening they seemed to be related to the same root cause

drewmullen commented 2 years ago

@alisdair on top of taking around 45m, it should also fail: https://github.com/drewmullen/terraform-aws-vpc/blob/88dce3e1320cd28febeaba63d141c3d928b9c957/tests/examples_ipam_vpc/tests.tf#L19

i tried to force a failure 2 ways, with regex for /29$ (should be 28) and then also just setting false

alisdair commented 2 years ago

from multiple-component perspective, perhaps its not a bug but from a user experience you should either:

  • error
  • warn the user that tests are being skipped

Agreed, this is definitely a sharp edge with the current testing experiment. We're aware that it's an issue, but unfortunately it isn't one we'll be able to address until the next iteration of testing. Some details here, in case you're interested: https://github.com/hashicorp/terraform/blob/6530055d188d363bf64ff99b56bb00b350de2cfb/internal/moduletest/provider.go#L270-L277

alisdair commented 2 years ago

Looking at the other repository you linked to, the issue appears to be a configuration which cannot be applied. If you change into the test directory and apply, it fails before reaching the assertions:

aws_vpc_ipam.example: Creating...
module.no_ipam_vpc_example.aws_vpc.this[0]: Creating...
module.no_ipam_vpc_example.aws_vpc.this[0]: Creation complete after 2s [id=vpc-03c65093c91773df1]
aws_vpc_ipam.example: Creation complete after 6s [id=ipam-0de3de0f95a17e2b2]
aws_vpc_ipam_pool.ipv4_example: Creating...
aws_vpc_ipam_pool.ipv4_example: Still creating... [10s elapsed]
aws_vpc_ipam_pool.ipv4_example: Creation complete after 13s [id=ipam-pool-0674aedcb97a12f0a]
aws_vpc_ipam_pool_cidr.ipv4_example: Creating...
module.ipv4_ipam_explicit_cidr_vpc.aws_vpc.this[0]: Creating...
module.ipv4_ipam_default_netmask_vpc.aws_vpc.this[0]: Creating...
module.ipv4_ipam_explicit_netmask_vpc.aws_vpc.this[0]: Creating...
aws_vpc_ipam_pool_cidr.ipv4_example: Still creating... [10s elapsed]
aws_vpc_ipam_pool_cidr.ipv4_example: Creation complete after 14s [id=172.2.0.0/16_ipam-pool-0674aedcb97a12f0a]
╷
│ Warning: The test provider is experimental
│
│   with provider["terraform.io/builtin/test"],
│ The Terraform team is using the test provider (terraform.io/builtin/test) as
│ part of ongoing research about declarative testing of Terraform modules.
│
│ The availability and behavior of this provider is expected to change
│ significantly even in patch releases, so we recommend using this provider
│ only in test configurations and constraining your test configurations to an
│ exact Terraform version.
╵
╷
│ Error: Error creating VPC: InvalidParameterValue: The allocation size is too big for the pool.
│   status code: 400, request id: 1e677b02-4056-434a-a40c-e94fe80727a5
│
│   with module.ipv4_ipam_explicit_netmask_vpc.aws_vpc.this[0],
│   on ../../main.tf line 29, in resource "aws_vpc" "this":
│   29: resource "aws_vpc" "this" {
│
╵
╷
│ Error: Error creating VPC: InvalidParameterValue: The allocation size is too big for the pool.
│   status code: 400, request id: 773515ec-41c3-4496-9366-536e31598a8d
│
│   with module.ipv4_ipam_explicit_cidr_vpc.aws_vpc.this[0],
│   on ../../main.tf line 29, in resource "aws_vpc" "this":
│   29: resource "aws_vpc" "this" {
│
╵
╷
│ Error: Error creating VPC: InvalidParameterValue: The allocation size is too big for the pool.
│   status code: 400, request id: 0b74936c-9b3b-4395-a781-5070bf9f0638
│
│   with module.ipv4_ipam_default_netmask_vpc.aws_vpc.this[0],
│   on ../../main.tf line 29, in resource "aws_vpc" "this":
│   29: resource "aws_vpc" "this" {
│

This is noted subtly in the documentation (see excerpt below), and is certainly another sharp edge. I'll retitle this issue accordingly and we should aim to fix this in the next iteration of terraform test.

The test_assertions resource captures any assertion failures but does not return an error, because that can then potentially allow downstream assertions to also run and thus capture as much context as possible. However, if Terraform encounters any errors while processing the test configuration it will halt processing, which may cause some of the test assertions to be skipped.

drewmullen commented 2 years ago

@alisdair thank you for pointing that out! the explicit depends_on for those modules got lost while moving around commits

I may open a feature request that terraform test drops a tfstate file upon failure. i finally got the error to fire appropriately and now i have a ton of dangling resources 🤣 thoughts?

alisdair commented 2 years ago

That seems like a good feature request to me, thanks in advance for filing it!

Dennizz commented 2 years ago

I'm having trouble understanding from this thread, what the expected behavior should be when resource creation fails. E.g. VPC creation fails, but test assertion is not run, would still test as successful? This is what I'm seeing at least.

If any resource creation fails, for whatever reason, even though test assertions have not taken place, I would like to see that reflected in the test result, as this could be an issue with the module code. Or should we test for such issues in another way?