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

proposal: function checks #412

Merged
merged 12 commits into from
Mar 14, 2025
Merged

Conversation

bschaatsbergen
Copy link
Member

@bschaatsbergen bschaatsbergen commented Jan 14, 2025

This is a simple prototype for adding new knownvalue checks, allowing a custom function to run against a plan or state value at a specific path within a statecheck. This approach builds on a suggestion from @jar-b, following my earlier proposal to extract a value from the state by traversing a path and referencing it elsewhere. It feels like a more elegant solution, addressing the need to enable users with the ability to perform custom validations on attributes during plan and apply checks.

Here’s an example of a StringFunc that assigns the underlying value of the given path to v:

func TestExpectKnownValue_CheckState_StringFunc(t *testing.T) {
	t.Parallel()

	resource.Test(t, resource.TestCase{
		// Provider definition omitted.
		Steps: []resource.TestStep{
			{
				// Example resource containing a string attribute named "configurable_attribute"
				Config: `resource "test_resource" "one" {}`,
				ConfigStateChecks: []statecheck.StateCheck{
					statecheck.ExpectKnownValue(
						"test_resource.one",
						tfjsonpath.New("configurable_attribute"),
						knownvalue.StringFunc(func(v string) error {
							if !strings.HasPrefix(v, "str") {
								return fmt.Errorf("value must start with 'str'")
							}
							return nil
						}),
					),
				},
			},
		},
	})
}

This makes it easier for provider developers to bring custom validation to these tests.

In this pull request you'll find a prototype for StringFunc and other functions. It's still work in progress and I'd appreciate your feedback and thoughts. Thanks!

@bschaatsbergen bschaatsbergen changed the title proposal: simplified value extraction using state checks proposal: value extraction Jan 14, 2025
@jar-b
Copy link
Member

jar-b commented Jan 15, 2025

Just an observer comment after thinking on this a bit from the provider perspective. I wonder if this problem could be solved via a more generic known value check that accepts a user defined function. For string types, the implementation could look something like:

// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue

import "fmt"

var _ Check = stringFunc{}

type stringFunc struct {
	checkFunc func(v string) error
}

// CheckValue determines whether the passed value is of type string, and
// returns no error from the provided check function
func (v stringFunc) CheckValue(other any) error {
	otherVal, ok := other.(string)

	if !ok {
		return fmt.Errorf("expected string value for StringFunc check, got: %T", other)
	}

	return v.checkFunc(otherVal)
}

// String returns the string representation of the value.
func (v stringFunc) String() string {
	// Validation is up the the implementer of the function, so there are no
	// string literal or regex comparers to print here
	return "stringFunc"
}

// StringFunc returns a Check for passing the string value in state
// to the provided check function
func StringFunc(fn func(v string) error) stringFunc {
	return stringFunc{
		checkFunc: fn,
	}
}

Where a test step could use the value pulled from state to do whatever custom validation is required.

                        knownvalue.StringFunc(func(s string) error {
                                // custom validation here
                                if s != "foo" {
                                        return fmt.Errorf("%s was not foo", s)
                                }
                                return nil
                        }),

I'm not sure if this is too generic for inclusion in the testing library itself. If so, this could be a custom known value check defined once in a provider, then re-used across test cases which bring their own validation functions as necessary.

@bschaatsbergen bschaatsbergen changed the title proposal: value extraction proposal: custom checks Jan 15, 2025
@bschaatsbergen bschaatsbergen changed the title proposal: custom checks proposal: custom (function) checks Jan 15, 2025
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up! Regardless of which approach is taken I think having some form of "extract this thing from this artifact" would be useful. Left some comments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jar-b @bschaatsbergen Would be interested to hear your thoughts on why use a knownvalue check over statecheck to execute this function. I have some assumptions, but it'd help me get a better idea of what you want to use it for!

Copy link
Member Author

@bschaatsbergen bschaatsbergen Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my initial prototype, I used statecheck functions to extract and assign a resource attribute value to a variable, and while that worked fine, it required a subtest to assert the value. It didn't feel in line with the existing test-writing DX that provider developers are familiar with. Added, referencing the variable in another check or a subsequent TestStep wasn’t really an option since those steps run in different contexts. So while the approach did the job, it felt a bit limiting.

Switching to knownvalue checks felt like a better fit—as these new functions expose the underlying value directly through the function signature, making it easier for users to define their own validation logic inside the check, instead of using a subtest. The actual assertion of the value also happens within the knownvalue check itself. Plus, it keeps everything nicely self-contained within the TestCase, which aligns well with the current API design.

One thing I really like about this approach is how it allows for simple, inline validation:

knownvalue.StringFunc(func(v string) error {
	if !strings.HasPrefix(v, "str") {
		return fmt.Errorf("value must start with 'str'")
	}
	return nil
})

Would love to hear your thoughts on this too, @austinvalle!

@bschaatsbergen bschaatsbergen marked this pull request as ready for review January 20, 2025 14:55
@bschaatsbergen bschaatsbergen requested review from a team as code owners January 20, 2025 14:55
@bschaatsbergen bschaatsbergen changed the title proposal: custom (function) checks proposal: function checks Jan 21, 2025
Copy link

@pbortnick pbortnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks fine from web presence perspective. Deferring to others for content approval.

@YakDriver
Copy link
Member

YakDriver commented Feb 14, 2025

I like this approach (combining the efforts of @jar-b and @bschaatsbergen). Nothing stands out as needing immediate change. A few related thoughts:

  1. Typed functions improve convenience – Repeated patterns benefit from typed functions. For perspective, the AWS Provider currently has 20,423 uses of prebuilt resource helper TestCheckFuncs. At that scale, convenience matters.
  2. Reducing regex reliance – As we transition to ConfigStateChecks and ConfigPlanChecks, this could also be an opportunity to reduce regex usage in validation. While regex is convenient, a simpler, more efficient check (e.g., strings.Contains() or strings.HasPrefix()) often suffices. Given the scale of the AWS Provider, regex overhead is becoming a concern.
  3. Improved testing through simplicity – Making thorough testing easier leads to better tests. Time constraints often favor quick and simple solutions over comprehensive ones. Efforts that streamline better practices will help raise overall test quality.
  4. A stepping stone – I suspect that the flexibility this effort introduces will, over time, help us identify additional prebuilt helpers to add.

@austinvalle
Copy link
Member

Thanks for the thoughts/review @YakDriver + @jar-b, I'll bring this PR up to my team's next triage meeting to discuss!

@austinvalle austinvalle added this to the v1.12.0 milestone Mar 14, 2025
@austinvalle austinvalle added the enhancement New feature or request label Mar 14, 2025
@austinvalle austinvalle self-assigned this Mar 14, 2025
It is unnecessary to copy loop variables because these variables have per-iteration scope instead of per-loop scope as it was in Go < 1.22.
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay on this PR review. Showed this to the team during our triage meeting and all agreed this is good to merge. If we need something similar for maps, lists, etc, we can introduce it later!

Also our docs have moved to https://github.com/hashicorp/web-unified-docs, but I'll take care of moving your changes over there as I'm not 100% sure how they want us to deal with new versions 🤔

@austinvalle austinvalle merged commit 96ff574 into hashicorp:main Mar 14, 2025
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants