Add additional import test modes

[stephybun]

Based off of #431
Supersedes #441

TODOs:

• Add additional test cases e.g. for ImportStateVerifyIgnore, SkipDataSource?
• Add logic to compare the import plan with the state from the previous test step

  terraform-plugin-testing/helper/resource/testing_new_import_state.go

  Line 216 in 805da5a

  // TODO compare plan to state from previous step

• Consider splitting this new import testing mode out into it's own file e.g. testing_new_plannable_import.go
• Enable these new import modes for configs supplied via file or directory
• Automatically add the terraform block to in-lined configs in the import step

  terraform-plugin-testing/helper/resource/testing_new_import_block_id_test.go

  Lines 445 to 451 in 1b47671

  	terraform {
  	required_providers {
  	examplecloud = {
  	source = "registry.terraform.io/hashicorp/examplecloud"
  	}
  	}
  	}

[stephybun]

It looks like this was only ever needed and done for the import tests, so I've opted to move this into testing_new_import_state.go and pass the appliedConfig through as a string so it can be easily appended/modified based on the import mode/kind we're using in the test.

[bbasata]

💯

[bbasata]

I wonder how this interacts with the earlier teststep.PrepareConfigurationRequest from step.Config on line 26. Would this change the behavior of any existing ImportCommandWithId tests that define step.Config? I actually don't know 😃

[stephybun]

I can confirm that this doesn't change the behaviour of existing ImportCommandWithId tests that define a Config in-line.

[bbasata]

Something I had overlooked in the proof-of-concept pull request: what is the significance of stepIndex + 1 here (and at line 26)?

[stephybun]

I believe it's to maintain the correct step number which was modified to index starting from 1 for a better UX

terraform-plugin-testing/helper/resource/testing_new.go

Line 144 in 7e59fd9

stepNumber = stepIndex + 1 // 1-based indexing for humans

This instance (as well as another in testing_new.go) can probably be updated to use stepNumber

[SBGoods]

We are only adding the import block to the config automatically if the ImportBlockWithId mode is set in a step without a config, is this expected? I wonder if we should always add the import block automatically to the config for ImportBlockWithId kinds.

Currently, you can run a test with ImportBlockWithId set with a config that does not have an import block:

{
  Config: `
  terraform {
  	  required_providers {
    	  examplecloud = {
      		  source = "registry.terraform.io/hashicorp/examplecloud"
    	  }
	  }
  }
  
  resource "examplecloud_container" "test" {
	  name = "somename"
	  password = "somevalue"
  }`,
ResourceName:            "examplecloud_container.test",
ImportState:             true,
ImportStateKind:         ImportBlockWithId,
ImportStateVerify:       true,
ImportStateVerifyIgnore: []string{"password"},
},

With the current implementation, this would be running a regular import test (not a block id test) but there is no indication to provider developers that that is the case.

[stephybun]

That is a good point, my thinking was purely based on how import tests have been used in the azurerm provider which is that we always re-use the config in the initial create test step so there is never need to specify Config for an import test. That assumption has been blown completely out of the water after looking into how tests using an import block function. Either way you've raised an important point and I think we should be appending the import block to a config that's been defined in the test step if ImportBlockWithId has been specified.

[stephybun]

The behaviour of these new modes is currently:

1. Generate and append an import block to the test configuration
2. Run apply instead of the import command

I don't think the testing framework should be modifying test configs defined in files or a directory. Thus the addition of this validation after thinking about this comment.

Longer term (maybe a subsequent PR) I think it's worth adding support for file, I'm just unsure whether it's wise to have a part of the behaviour differ depending on how the config is provided, i.e., if config were provided by a file the import kind should no longer do 1. but only 2..

[bbasata]

I noticed a nice side-effect here. ImportCommandWithId equals zero. So, all existing tests will default to ImportCommandWithId, as TestStep.ImportStateKind will get the zero value.

[bbasata]

This is very ready to merge 😃

• Feature toggle: the PR adds a work-in-progress, optional pre-release feature; this functionality is only activated when the test author sets a non-zero (i.e. non-default) value on TestStep.ImportStateKind.

• Regression-safety: the refactoring to support the new feature looks safe, and this module's tests are passing.

We'll iterate on this feature in follow-up PRs.

This was a joy, @stephybun ✨