Skip to content

Testing alignment

Description

Looking at our test code, we have a number of different testing approaches that vary by resource. 😩 As long as new PRs are looking to existing testing patterns in the codebase, I think we should work to model good testing, so I'm hoping to get some feedback before doing some refactoring. 💃

@timofurrer and I have discussed what we feel is a good approach for new tests. (Please jump in if I misrepresent anything Timo!) The theme is to make them simpler to write and review. We can delete hundreds of lines of test code per resource.

  1. The test config should only include the resource/s being tested.

    Undesirable:

    resource "gitlab_project" "foo" {
  name = "foo-%d"
  description = "Terraform acceptance tests"

  # So that acceptance tests can be run in a gitlab organization
  # with no billing
  visibility_level = "public"
}

resource "gitlab_managed_license" "foo" {
  project = "${gitlab_project.foo.id}"
  name = "MIT"
  approval_status = "%s"
}

    Desirable:

    resource "gitlab_managed_license" "foo" {
  project = %d
  name = "MIT"
  approval_status = "%s"
}

    Pros:

    • Allows us to use CheckDestroy correctly (see #205 (closed))
    • Less code to review
    • Isolates issues
    • Faster runtime (fewer API calls)

    Cons:

    • Requires a single call to testAccCheck(t) at the top of the test function when using helpers to set up the test using the GitLab client. EDIT: There have been a couple suggestions on how to avoid this problem, like using TestMain.

    Unknowns:

    • I haven't considered whether we should do this for data sources as well. With data sources there's high variability in the set-up API calls you would need to make (one per data source type). EDIT: We decided in the discussion we would do this for both resources and data sources.

  2. Use ImportStateVerify TestSteps as a replacement for writing custom "CheckExists" and "CheckAttributes" check funcs.

    Undesirable:

    {
	Config: testAccGitlabFooConfig(rInt),
	Check: resource.ComposeTestCheckFunc(
		testAccCheckGitlabFooExists("gitlab_foo.foo", &foo),
		testAccCheckGitlabFooAttributes(&foo, &testAccGitlabFooExpectedAttributes{
			Foo: "bar",
			Baz:  123,
		}),
	),
},

    Desirable:

    {
	Config: testAccGitlabFooConfig(rInt),
	// Check computed and default attributes.
	Check: resource.ComposeTestCheckFunc(
		resource.TestCheckResourceAttrSet("gitlab_foo.foo", "bar"),
	),
},
{
	ResourceName:      "gitlab_foo.foo",
	ImportState:       true,
	ImportStateVerify: true,
},

    The reason this works is because a) the test framework automatically re-runs terraform plan after each step and fails if the plan is not empty for the same config (source), and b) using ImportStateVerify checks that the state after importing matches the state before importing, bypassing any DiffSuppressFunc or CustomizeDiff (source). This should accomplish the same thing as checking the state and upstream resource manually.

    Pros:

    • No need to write a dedicated "CheckExists" function for each resource
    • No need to write a dedicated "CheckAttributes" function for each resource (a common sore spot for copy-paste bugs and neglecting new attributes)
    • Encourages pragmatic testing using the framework's builtin check functions where necessary, especially for checking computed attributes which we routinely neglect
    • Faster runtime (fewer API calls)

    Cons:

    • None????? At least not for most routine kinds of tests we have.

    Unknowns:

    • This change feels radical, so I'm interested in if anyone sees flaws with it. It feels weird deleting a large amount of code, but in most cases these extra check functions don't seem to be doing anything. Maybe there are edge cases where they are useful.

  3. Prefer writing config in-line to wrapping it in a function.

    Undesirable:

    Steps: []resource.TestStep{
	{
		Config: testAccGitlabInstanceVariableConfig(rString),
		Check: resource.ComposeTestCheckFunc( // snip

func testAccGitlabInstanceVariableConfig(rString string) string {
	return fmt.Sprintf(`
resource "gitlab_instance_variable" "foo" {
  key           = "key_%[1]s"
  value         = "value-%[1]s"
  variable_type = "file"
  masked        = false
}
`, rString)
}

    Desirable:

    Steps: []resource.TestStep{
	{
		Config: fmt.Sprintf(`
		resource "gitlab_instance_variable" "foo" {
		  key           = "key_%[1]s"
		  value         = "value-%[1]s"
		  variable_type = "file"
		  masked        = false
		}`, rString),
		Check: resource.ComposeTestCheckFunc( // snip

    Of course there are exceptions for large resource configs or places where there is a lot of config reuse. But most frequently right now we give each config a new function, with no reuse.

    Pros:

    • Easier to review, when the config itself is beside the checks

    Cons:

    • Can become harder to follow for larger configs. At the high end it may still make sense to use a separate function.

*Heave* ...and then finally I should mention that for other testing pointers we would defer to HashiCorp Terraform testing best practices.

Here are some example tests that model the changes I've described:

How do we feel?

Completion Checklist

  • Use TestMain to gate acceptance tests instead of calling testAccCheck(t) in every test
  • Update the CONTRIBUTING.md testing guidance with a model test file
  • Refactor existing tests (break down in next section)

Refactors

Edited by Timo Furrer