Add Option to use Terraform Configuration in TestStep #153

bendbennett · 2023-07-17T16:46:00Z

Closes: #150

This PR adds the following fields to TestStep:

ConfigDirectory
ConfigFile
ConfigVariable

ConfigDirectory

Allows the specification of a path to a directory containing Terraform configuration files. All files in the directory are copied to the working directory used during the execution of an acceptance test TestStep.

ConfigFile

Allows the specification of a path to a file containing Terraform configuration. The file is copied to the working directory used during the execution of an acceptance test TestStep.

ConfigVariables

Allows defining Terraform variables for use in conjunction with Terraform configuration used during the execution of an acceptance test TestStep.

…s of TestStep.Config or TestStep.Directory can be encapsulated in teststep.config struct (#150)

…ation (#150)

…instantiating implementations of teststep.Config (#150)

…p.Config interface for applied configuration (#150)

…lock and TestStep.mergedConfig() to configuration.HasProviderBlock(), configuration.hasTerraformBlock and configuration.MergedConfig(), respectively (#150)

…configuration struct to simplify processing during GetRaw() and the equivalent function(s) for processing Directory and File (#150)

… wd.baseDir (#150)

…gement of writing test case provider config or test step provider config simpler when handling copying of files from configuration.directory (#150)

…Case or TestStep are being used when TestStep.ConfigDirectory is specified (#150)

…f provider blocks in configuration files within the configuration directory (#150)

…ally to configuration struct. (#150)

…ined within testCaseProviderConfig and testStepProvider config fields) when processing config directory as the expectation is that external providers will be specified directly in the terraform configuration files within the config directory (#150)

… validate that when ConfigDirectory is defined that ExternalProviders cannot be specified for either TestCase or TestStep (#150)

…blocks are only written when using TestStep.Config. The expectation is that when using TestStep.ConfigDirectoy, the terraform files within the configuration directory will specify the terraform and/or provider blocks as necessary (#150)

…aform configuration files in TestStep.ConfigDirectory (#150)

…ry (#150)

…ory() and TestStepDirectory() (#150)

…estStepConfigFunc (#150)

…n a TestStep (#150)

bflad

Nice work so far, @bendbennett! I left some initial thoughts here. I really like where you are going with the new configuration handling interface and methods since that will help clean up the testing logic implementation.

config/variable.go

bflad · 2023-07-24T15:12:21Z

config/variable.go

+		innerErr := err
+
+		for errors.Unwrap(innerErr) != nil {
+			innerErr = errors.Unwrap(err)
+		}
+
+		return nil, innerErr


Could you add a quick code comment about why this unwrapping is necessary? Thanks!

Have added a comment. Happy to expand/reduce etc.

bflad · 2023-07-24T15:59:02Z

internal/teststep/config.go

+}
+
+func Configuration(req ConfigurationRequest) (configuration, error) {
+	var populatedConfig []string


Nit: Can we just directly check the fields rather than storing them in a separate slice?

if req.Directory != nil && req.Raw != nil { // return error } if req.Directory != nil { return configuration{/* ... */}, nil } return configuration{/* ... */}, nil

Could potentially introduce a ConfigurationRequest.Validate() method to simplify further.

Have added a ConfigurationRequest.Validate() method as per your suggestion.

bflad · 2023-07-24T16:04:02Z

internal/teststep/config.go

+	return nil
+}
+
+func copyFile(path string, dstPath string) error {


internal/plugintest has some helpers for this sorta thing -- if it makes more sense, maybe that functionality can split into its own internal file handling package to help prevent duplicating logic, but totally optional of course.

I've left this as is for now if that's ok? Once these changes are merged I'll add an issue to consider refactoring.

bflad · 2023-07-24T16:05:24Z

internal/teststep/config.go

+	switch {
+	case c.hasRaw(ctx):
+		return providerConfigBlockRegex.MatchString(c.raw), nil
+	case c.hasDirectory(ctx):


💡 One thing you could do here to avoid checking each case repetitively introduce two separate configuration implementations -- ConfigurationDirectory and ConfigurationString

I've added configurationDirectory and configurationString types that implement the Config interface. The Configuration() constructor function now returns Config interface to accommodate the various types we will need (e.g., configurationDirectory, configurationString, configurationFile). Owing to the switch to returning an interface from the Configuration() constructor function, nil checks have been added.

config/directory.go

Co-authored-by: Brian Flad <bflad417@gmail.com>

austinvalle

So much great work Ben! 🚀 - Couple notes and one thought here:

With the amount of file/path specific tests we have now in plugin-testing, should we update our CI to run on other platforms as well (mac/windows)?

helper/resource/teststep_providers_test.go

internal/plugintest/util.go

internal/teststep/directory_test.go

website/docs/plugin/testing/acceptance-tests/configuration.mdx

…amically to determine the directory containing the Terraform configuration (#150)

bendbennett · 2023-07-31T09:23:42Z

With the amount of file/path specific tests we have now in plugin-testing, should we update our CI to run on other platforms as well (mac/windows)?

We could certainly consider this. Couple of observations when playing around with this:

Updating the OS matrix in ci-go.yml to use windows-latest and macos-latest results in the error below when running go test -coverprofile=coverage.out ./... on Windows. This can be handled by changing the running of the tests to something like go test ./... -v -cover:

no required module provides package .out; to add it: .........

Next up, there's an error generated for the TestTest_TestStep_ProviderFactories_CopyWorkingDir_EachTestStep test that's specific to MacOS 12. This can be handled by using macos-13 in the testing matrix, but will require that we circle back round and update to macos-latest once the default MacOS for GitHub actions has been bumped to 13.
Finally, the tests that purposely fail to find a directory/file, for instance, TestConfigurationFile_HasProviderBlock "not-file", will need to be updated as the error message on Windows differs from Ubuntu and MacOS (e.g., The system cannot find the file specified versus no such file or directory).

I'm happy to make these changes if everyone is on board with the above or some variant thereof.

bflad

Drive-by question (sorry I'm having trouble getting time to review this properly): Is there something handling the removal of configuration files not present when there are multiple ConfigDirectory steps?

e.g. if first test step has ConfigDirectory with a.tf and b.tf.json and second test step has ConfigDirectory has c.tf and d.tf.json, the working directory should only have c.tf and d.tf.json during the second test step.

bflad · 2023-07-26T21:00:03Z

config/directory.go

+// TestStepConfigRequest defines the request supplied to types
+// implementing TestStepConfigFunc.
+type TestStepConfigRequest struct {
+	StepNumber int


It might be helpful to document that this number is one-based for humans instead of the underlying Go slice index which is zero-based.

Have updated the docs to this effect.

bflad · 2023-07-26T21:01:42Z

config/directory.go

+// these files within a directory with the same name as the test.
+func TestStepDirectory() func(TestStepConfigRequest) string { //nolint:paralleltest //Not a test


Similar to above 😉

Suggested change

// these files within a directory with the same name as the test.

func TestStepDirectory() func(TestStepConfigRequest) string { //nolint:paralleltest //Not a test

// these files within a directory with the same name as the test.

//

// For example, given test code:

//

// func TestExampleCloudThing_basic(t *testing.T) {

// resource.Test(t, resource.TestCase{

// Steps: []resource.TestStep{

// {

// ConfigDirectory: config.TestStepDirectory(),

// },

// },

// })

// }

//

// The testing configurations will be expected in the

// testdata/TestExampleCloudThing_basic/1/ directory.

func TestStepDirectory() func(TestStepConfigRequest) string {

Have updated the docs to this effect.

austinvalle · 2023-07-31T13:25:54Z

helper/resource/teststep_providers_test.go

+// TestTest_ConfigDirectory_TestStepDirectory_StepNotHardcoded uses a multistep test
+// to prove that the test step number is not hardcoded


austinvalle · 2023-07-31T13:55:12Z

With the amount of file/path specific tests we have now in plugin-testing, should we update our CI to run on other platforms as well (mac/windows)?

We could certainly consider this. Couple of observations when playing around with this:

Updating the OS matrix in ci-go.yml to use windows-latest and macos-latest results in the error below when running go test -coverprofile=coverage.out ./... on Windows. This can be handled by changing the running of the tests to something like go test ./... -v -cover:
no required module provides package .out; to add it: .........
Next up, there's an error generated for the TestTest_TestStep_ProviderFactories_CopyWorkingDir_EachTestStep test that's specific to MacOS 12. This can be handled by using macos-13 in the testing matrix, but will require that we circle back round and update to macos-latest once the default MacOS for GitHub actions has been bumped to 13.

Finally, the tests that purposely fail to find a directory/file, for instance, TestConfigurationFile_HasProviderBlock "not-file", will need to be updated as the error message on Windows differs from Ubuntu and MacOS (e.g., The system cannot find the file specified versus no such file or directory).

I'm happy to make these changes if everyone is on board with the above or some variant thereof.

Hmm 🤔 , I don't want it to be too big of a pain... Maybe we just skip running them for different OS's for now? If we start running into bug reports that are OS specific we can revisit. I'd imagine the amount of users running into Window specific bugs is limited for Go modules like this anyways 🤷🏻 . Thanks for giving it a go!

…s prior to copy configuration and variables for current test step (#150)

bendbennett · 2023-07-31T16:22:01Z

Drive-by question (sorry I'm having trouble getting time to review this properly): Is there something handling the removal of configuration files not present when there are multiple ConfigDirectory steps?

e.g. if first test step has ConfigDirectory with a.tf and b.tf.json and second test step has ConfigDirectory has c.tf and d.tf.json, the working directory should only have c.tf and d.tf.json during the second test step.

Good point! Have added something to handle removal of config and variables files from previous test step prior to running the current test step along with some test coverage.

bflad

Overall looks good to me 🚀

bflad · 2023-08-25T20:43:54Z

.changes/unreleased/FEATURES-20230728-151123.yaml

I'm a little torn on whether we should "feature" changelog all the individual config package functionality, since the above changelog entry covers the addition and purpose of the whole config package. Generally "feature" should be reserved for major functionality. Personally I think the above entry and individual helper/resource changes do fit the criteria for major functionality. 👍

.changes/unreleased/FEATURES-20230728-152737.yaml

.changes/unreleased/FEATURES-20230728-152822.yaml

.changes/unreleased/FEATURES-20230728-152917.yaml

config/directory.go

config/file.go

helper/resource/testcase_validate.go

helper/resource/testing_new.go

bflad · 2023-08-25T21:00:48Z

internal/plugintest/working_dir.go

+
+	for _, file := range fi {
+		if file.Mode().IsRegular() {
+			if filepath.Ext(file.Name()) == ".tf" || filepath.Ext(file.Name()) == ".json" {


Open question: Not sure if we should or shouldn't be concerned about non-Terraform JSON files here.

I think that the only .json files within the working_dir will be those that are copied when a .json file, or a directory containing .json files is specified for TestStep.ConfigFile, orTestStep.ConfigDirectory, respectively. So I believe removing .json files will be ok from this perspective.

Do you know any way in which a non-Terraform JSON file might end-up in the working directory, and if so, whether removing it would be likely to cause issues?

Hmm, I think you're right to a certain degree. Maybe I was thinking of the opposite problem then -- let's say we're using ConfigDirectory across multiple steps and I have:

# step 1 directory |- file.whateverextension \ main.tf # step 2 directory \ main.tf

We should be preserving things necessary between steps in the working directory such as the terraform.tfstate, but removing the rest of the working directory files/directories. The configuration copying would overwrite any exactly named files between steps, so in that regard its okay, but I think it would issue if the configuration was using something like fileset() instead of referencing thing with file() directly and expecting those lingering files to be removed between steps. Let's treat this as something to follow up with rather than holding this up further though!

Thanks for the additional context. Have created a follow-up issue - Consider removal of all files copied from ConfigDirectory between sequential test steps

Co-authored-by: Brian Flad <bflad417@gmail.com>

bflad · 2023-08-30T16:37:27Z

internal/plugintest/working_dir.go

+
+	for _, file := range fi {
+		if file.Mode().IsRegular() {
+			if filepath.Ext(file.Name()) == ".tf" || filepath.Ext(file.Name()) == ".json" {


Hmm, I think you're right to a certain degree. Maybe I was thinking of the opposite problem then -- let's say we're using ConfigDirectory across multiple steps and I have:

# step 1 directory |- file.whateverextension \ main.tf # step 2 directory \ main.tf

We should be preserving things necessary between steps in the working directory such as the terraform.tfstate, but removing the rest of the working directory files/directories. The configuration copying would overwrite any exactly named files between steps, so in that regard its okay, but I think it would issue if the configuration was using something like fileset() instead of referencing thing with file() directly and expecting those lingering files to be removed between steps. Let's treat this as something to follow up with rather than holding this up further though!

github-actions · 2023-10-01T02:05:47Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bendbennett added 28 commits July 17, 2023 09:36

Adding ConfigDirectory to TestStep (#150)

0b590e7

Add teststep.Config interface so that logic that examines the content…

91b0ce7

…s of TestStep.Config or TestStep.Directory can be encapsulated in teststep.config struct (#150)

Update TestStep.Validate() to use teststep.Config interface implement…

3937312

…ation (#150)

Switching to using a teststep.ConfigurationRequest struct for use in …

6838303

…instantiating implementations of teststep.Config (#150)

Refactoring to use type implementing teststep.Config interface (#150)

5634313

Refactoring testStepNewImportState() to use type implementing testste…

18a3602

…p.Config interface for applied configuration (#150)

Switching to using type implementing teststep.Config interface (#150)

f9f0b1b

Moved TestStep.configHasProviderBlock(), TestStep.configHasTerraformB…

1e1b53c

…lock and TestStep.mergedConfig() to configuration.HasProviderBlock(), configuration.hasTerraformBlock and configuration.MergedConfig(), respectively (#150)

Fix validation error message to include ConfigDirectory (#150)

da96c02

Added testCaseProviderConfig and testStepProviderConfig as fields on …

4230bb1

…configuration struct to simplify processing during GetRaw() and the equivalent function(s) for processing Directory and File (#150)

Add initial implementation of copying files from ConfigDirectory into…

d46d517

… wd.baseDir (#150)

Switch to using WriteDirectory() method on configuration to make mana…

80c09f5

…gement of writing test case provider config or test step provider config simpler when handling copying of files from configuration.directory (#150)

Adding tests to verify that the ExternalProviders specified in a Test…

516e68b

…Case or TestStep are being used when TestStep.ConfigDirectory is specified (#150)

Extending implementation of HasProviderBlock() to include detection o…

6a8f3da

…f provider blocks in configuration files within the configuration directory (#150)

Moving writing of raw config or copy of config directory files intern…

6e04f0c

…ally to configuration struct. (#150)

Adding HasConfigurationFiles() func to Config interface to be able to…

0966142

… validate that when ConfigDirectory is defined that ExternalProviders cannot be specified for either TestCase or TestStep (#150)

Adding a couple of tests to verify behaviour when using multiple terr…

a800d8e

…aform configuration files in TestStep.ConfigDirectory (#150)

Switching to using TestStepConfigFunc type for TestStep.ConfigDirecto…

f07f6aa

…ry (#150)

Adding acceptance test coverage for StaticDirectory(), TestNameDirect…

2877b6b

…ory() and TestStepDirectory() (#150)

Adding Exec() method to TestStepConfigFunc type and removing ExecuteT…

ea3044c

…estStepConfigFunc (#150)

Adding ConfigVariables to allow defining of Terraform variables withi…

7b0ac16

…n a TestStep (#150)

Adding docs for config directory and variables (#150)

4aa11b7

Adding tests for TestStepConfigFunc.Exec() method (#150)

136e47e

Adding tests for config<Type>Variable and Variables.Write() func (#150)

a98a036

Adding copyright headers (#150)

88066ac

Unmarshalling before comparing (#150)

36409db

bflad reviewed Jul 24, 2023

View reviewed changes

Apply suggestions from code review

3f6a544

Co-authored-by: Brian Flad <bflad417@gmail.com>

bendbennett marked this pull request as ready for review July 28, 2023 15:31

bendbennett requested a review from a team as a code owner July 28, 2023 15:31

austinvalle approved these changes Jul 28, 2023

View reviewed changes

bendbennett force-pushed the bendbennett/issues-150 branch from 03ec329 to 8191444 Compare July 31, 2023 07:44

bendbennett added 3 commits July 31, 2023 09:37

Adding test to demonstrate that the TestStep number is being used dyn…

a95efe7

…amically to determine the directory containing the Terraform configuration (#150)

Removing unneeded elsif conditional (#150)

711dac6

Updating docs (#150)

3197257

bendbennett force-pushed the bendbennett/issues-150 branch from f2e79ac to 3197257 Compare July 31, 2023 09:03

bendbennett force-pushed the bendbennett/issues-150 branch from 99ebc7e to 3197257 Compare July 31, 2023 09:28

bflad reviewed Jul 31, 2023

View reviewed changes

austinvalle reviewed Jul 31, 2023

View reviewed changes

bendbennett added 2 commits July 31, 2023 17:12

Removing any configuration or variables files from previous test step…

a8cab58

…s prior to copy configuration and variables for current test step (#150)

Updating docs (#150)

48fabe2

Merge remote-tracking branch 'origin/main' into bendbennett/issues-150

cba81dc

bflad approved these changes Aug 25, 2023

View reviewed changes

bendbennett and others added 5 commits August 29, 2023 09:39

Apply suggestions from code review

2117343

Co-authored-by: Brian Flad <bflad417@gmail.com>

Update .changes/unreleased/FEATURES-20230728-152822.yaml

29dec70

Co-authored-by: Brian Flad <bflad417@gmail.com>

Removing unneeded changelog entries (#150)

76b0526

Reusing stepNumber variable (#150)

9f31393

Merge remote-tracking branch 'origin/main' into bendbennett/issues-150

19c39ec

bflad approved these changes Aug 30, 2023

View reviewed changes

bendbennett mentioned this pull request Aug 31, 2023

Consider removal of all files copied from ConfigDirectory between sequential test steps #172

Open

bendbennett merged commit e46c64f into main Aug 31, 2023
5 checks passed

bendbennett deleted the bendbennett/issues-150 branch August 31, 2023 06:15

bendbennett mentioned this pull request Sep 7, 2023

helper/resource: Support Configuration Directories and Variables in Acceptance Testing hashicorp/terraform-plugin-sdk#620

Closed

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Option to use Terraform Configuration in TestStep #153

Add Option to use Terraform Configuration in TestStep #153

bendbennett commented Jul 17, 2023 •

edited

Loading

bflad left a comment

bflad Jul 24, 2023

bendbennett Jul 26, 2023

bflad Jul 24, 2023

bendbennett Jul 26, 2023

bflad Jul 24, 2023

bendbennett Jul 26, 2023

bflad Jul 24, 2023

bendbennett Jul 26, 2023

austinvalle left a comment

bendbennett commented Jul 31, 2023 •

edited

Loading

bflad left a comment

bflad Jul 26, 2023 •

edited

Loading

bendbennett Jul 31, 2023

bflad Jul 26, 2023

bendbennett Jul 31, 2023

austinvalle Jul 31, 2023

austinvalle commented Jul 31, 2023 •

edited

Loading

bendbennett commented Jul 31, 2023

bflad left a comment

bflad Aug 25, 2023

bflad Aug 25, 2023

bendbennett Aug 29, 2023

bflad Aug 30, 2023 •

edited

Loading

bendbennett Aug 31, 2023

bflad Aug 30, 2023 •

edited

Loading

github-actions bot commented Oct 1, 2023

		// these files within a directory with the same name as the test.
		func TestStepDirectory() func(TestStepConfigRequest) string { //nolint:paralleltest //Not a test

-// these files within a directory with the same name as the test.
-func TestStepDirectory() func(TestStepConfigRequest) string { //nolint:paralleltest //Not a test
+// these files within a directory with the same name as the test.
+//
+// For example, given test code:
+//
+//	func TestExampleCloudThing_basic(t *testing.T) {
+//	    resource.Test(t, resource.TestCase{
+//	        Steps: []resource.TestStep{
+//	            {
+//	                ConfigDirectory: config.TestStepDirectory(),
+//	            },
+//	        },
+//	    })
+//	}
+//
+// The testing configurations will be expected in the
+// testdata/TestExampleCloudThing_basic/1/ directory.
+func TestStepDirectory() func(TestStepConfigRequest) string {

		// TestTest_ConfigDirectory_TestStepDirectory_StepNotHardcoded uses a multistep test
		// to prove that the test step number is not hardcoded

Add Option to use Terraform Configuration in TestStep #153

Add Option to use Terraform Configuration in TestStep #153

Conversation

bendbennett commented Jul 17, 2023 • edited Loading

ConfigDirectory

ConfigFile

ConfigVariables

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle left a comment

Choose a reason for hiding this comment

bendbennett commented Jul 31, 2023 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle commented Jul 31, 2023 • edited Loading

bendbennett commented Jul 31, 2023

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 1, 2023

bendbennett commented Jul 17, 2023 •

edited

Loading

bendbennett commented Jul 31, 2023 •

edited

Loading

bflad Jul 26, 2023 •

edited

Loading

austinvalle commented Jul 31, 2023 •

edited

Loading

bflad Aug 30, 2023 •

edited

Loading

bflad Aug 30, 2023 •

edited

Loading