Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helper/resource: Checking Collection Attribute Length Ignores Missing/Null Attributes #1085

Open
bflad opened this issue Oct 27, 2022 · 1 comment
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.

Comments

@bflad
Copy link
Contributor

bflad commented Oct 27, 2022

SDK version

v2.24.0

Relevant provider source code

// sdk resource logic
d.Set("list", nil) // schema.TypeList
d.Set("map", nil) // schema.TypeMap
d.Set("set", nil) // schema.TypeSet

// framework resource logic
ListNull(/* ... */)
MapNull(/* ... */)
SetNull(/* ... */)

// acceptance testing TestCheckFunc
resource.TestCheckResourceAttr("examplecloud_thing.test", "list.#", "0")
resource.TestCheckResourceAttr("examplecloud_thing.test", "map.%", "0")
resource.TestCheckResourceAttr("examplecloud_thing.test", "set.#", "0")

Terraform Configuration Files

resource "examplecloud_thing" "example" {
  # attributes not configured
}

Expected Behavior

Test checks fail that the attribute doesn't exist with the framework code.

Actual Behavior

Test checks pass with the framework code. It specifically ignores returning an error in this situation:

v, ok := is.Attributes[key]
if !ok {
// Empty containers may be elided from the state.
// If the intent here is to check for an empty container, allow the key to
// also be non-existent.
if value == "0" && (strings.HasSuffix(key, ".#") || strings.HasSuffix(key, ".%")) {
return nil
}

This rule was intentionally added because provider developers were already depending on this type of testing logic years ago:

57c8f96

Steps to Reproduce

  1. TF_ACC=1 go test -count=1 -v ./...

References

@bflad bflad added bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework. labels Oct 27, 2022
@bflad bflad changed the title helper/resource: Checking Collection Attribute Length Ignores Null Attributes helper/resource: Checking Collection Attribute Length Ignores Missing/Null Attributes Oct 27, 2022
@bflad
Copy link
Contributor Author

bflad commented Oct 27, 2022

Workarounds

Either of these options should workaround the current behavior and yield failing checks:

  1. Use resource.TestCheckResourceAttrSet("...", "....%") before or resource.TestCheckNoResourceAttr("...", "....%") in place of the 0 size check.
  2. Extract the map values from each flatmap state and compare for differences
func TestAccThingDataSource(t *testing.T) {
	var value1, value2 map[string]string

	resource.ParallelTest(t, resource.TestCase{
		Steps: []resource.TestStep{
			{
				// ... other fields
				Check: ComposeAggregateTestCheckFunc(
					testExtractResourceAttrMap("...", "map", &value1),
				),
			},
			{
				// ... other fields
				Check: ComposeAggregateTestCheckFunc(
					testExtractResourceAttrMap("...", "map", &value2),
					func(s *terraform.State) error {
						// this uses go-cmp, but reflect.DeepEqual() is probably fine too
						if diff := cmp.Diff(value1, value2); diff != "" {
							return fmt.Errorf("attribute value difference: %s", diff)
						}

						return nil
					},
				),
			},
		},
	})
}

func testExtractResourceAttrMap(resourceName string, attributeName string, attributeValue *map[string]string) TestCheckFunc {
	return func(s *terraform.State) error {
		rs, ok := s.RootModule().Resources[resourceName]

		if !ok {
			return fmt.Errorf("resource name %s not found in state", resourceName)
		}

		if _, ok := rs.Primary.Attributes[attributeName]; ok {
			return fmt.Errorf("attribute name %s found, but is not a map attribute in state", attributeName)
		}

		// Everything in a map will be in the flatmap with the attribute name,
		// a period, then either the size sigil or a string key.
		attributeValuePathPrefix := attributeName + "."
		attributePathSize := attributeValuePathPrefix + "%"

		sizeStr, ok := rs.Primary.Attributes[attributePathSize]

		if !ok {
			// attribute not found
			*attributeValue = nil

			return nil
		}

		size, err := strconv.Atoi(sizeStr)

		if err != nil {
			return fmt.Errorf("unable to convert map size %s to integer: %s", sizeStr, err)
		}

		m := make(map[string]string, size)

		for attributePath, attributePathValue := range rs.Primary.Attributes {
			// Ignore
			if !strings.HasPrefix(attributePath, attributeValuePathPrefix) {
				continue
			}

			if attributePath == attributePathSize {
				continue
			}

			key := strings.TrimPrefix(attributePath, attributeValuePathPrefix)

			m[key] = attributePathValue
		}

		*attributeValue = m

		return nil
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
Development

No branches or pull requests

1 participant