Consider improving error messaging for `knownvalue.NumberExact` float comparisons

[austinvalle]

terraform-plugin-testing version

v1.7.0

Use cases

When using knownvalue.NumberExact, it's possible to get confusing error messages in the situation where the provided *big.Float does not match the value parsed from a json.Number because of the mantissa using something like big.NewFloat.

Example

func TestNumberFunction_recreate(t *testing.T) {
    resource.UnitTest(t, resource.TestCase{
        ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
            "framework": providerserver.NewProtocol6WithError(New()),
        },
        Steps: []resource.TestStep{
            {
                Config: `output "test" {
                    value = 1.23
                }`,
                ConfigStateChecks: []statecheck.StateCheck{
                    statecheck.ExpectKnownOutputValue("test", knownvalue.NumberExact(big.NewFloat(1.23))),
                },
            },
        },
    })
}

Test error

--- FAIL: TestNumberFunction_recreate (0.43s)
    /Users/austin.valle/code/terraform-provider-corner/internal/framework6provider/number_function_test.go:80: Step 1/1 error: Post-apply refresh state check(s) failed:
        error checking value for output at path: test, err: expected value 1.23 for NumberExact check, got: 1.23
FAIL
FAIL	github.com/hashicorp/terraform-provider-corner/internal/framework6provider	0.896s
FAIL

---

What can make this more confusing is when it accidentally is correct and the test passes 😅 :

func TestNumberFunction_successful(t *testing.T) {
    resource.UnitTest(t, resource.TestCase{
        ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
            "framework": providerserver.NewProtocol6WithError(New()),
        },
        Steps: []resource.TestStep{
            {
                Config: `output "test" {
                    value = 1234.5
                }`,
                ConfigStateChecks: []statecheck.StateCheck{
                    // Success!
                    statecheck.ExpectKnownOutputValue("test", knownvalue.NumberExact(big.NewFloat(1234.5))),
                },
            },
        },
    })
}

Workaround

You can workaround this by creating the *big.Float exactly how the internal state check logic/Terraform creates it. Fixing the initial example in this issue:

func TestNumberFunction_recreate(t *testing.T) {
    matchingFloat, _, _ := big.ParseFloat("1.23", 10, 512, big.ToNearestEven)

    resource.UnitTest(t, resource.TestCase{
        ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
            "framework": providerserver.NewProtocol6WithError(New()),
        },
        Steps: []resource.TestStep{
            {
                Config: `output "test" {
                    value = 1.23
                }`,
                ConfigStateChecks: []statecheck.StateCheck{
                    // Success!
                    statecheck.ExpectKnownOutputValue("test", knownvalue.NumberExact(matchingFloat)),
                },
            },
        },
    })
}

Proposal

Improve the error message to also include the precision/rounding/mantissa information to make it more apparent why two floating point numbers don't match. Perhaps we could also make a suggestion about how best to create the *big.Float value if a comparison doesn't match because of the mantissa.

References

• Found while writing dynamic tests in: Add dynamic resource and dynamic function parameter/return tests terraform-provider-corner#228

[bflad]

Not in any way saying these are better, but a few other potential approaches here might be:

• Using (*big.Float).Text('f', -1) for number comparisons, since the internal *big.Float details differing may not necessarily matter to the intention of the assertion
• During the check logic, checking if the given *big.Float precision matches 512 bit precision and raising an error if not -- ostensibly this might be pretty annoying for developers though unless we potentially offer a helper
• During the check creation/logic, ensuring the precision matches 512, potentially by calling (*big.Float).SetPrec(512) -- I don't think we should have to worry about modifying the asserted value in this case unless the precision was actually given as higher and doing this would lower it (maybe then it could raise an error) although I think we would want to verify in these cases if the given value could already be "wrong"/unequal even if we "fix" the precision.

Curious what you think!

[austinvalle]

My biggest sticking point would be altering the general expectation that the input value to knownvalue.NumberExact wasn't "exactly" used in comparisons, although I'm not sure if we already do something like that in plugin framework 🤔 .

Developers who aren't worried about the precision could just compare it with a knownvalue.Float64Exact, although that might not match with the schema 😕 .

Would be interesting to hear more from provider developers attempting to use this and their expectations

[skyzyx]

This page was the only result when looking for the error message.

Step 1/1 error: Post-apply refresh state check(s) failed:

I'm having trouble comparing what I believed were identical strings.

locals {
  toml = <<EOT
abc = 123
EOT
}

output "as_json" {
  value = provider::corefunc::toml_to_json(local.toml)
}

#=> {"abc":123}

ConfigStateChecks: []statecheck.StateCheck{
    statecheck.ExpectKnownOutputValue(
        "as_json",
        knownvalue.StringExact(strings.TrimSpace(tc.Expected)),
    ),
},

Error message is:

error checking value for output at path: as_json, err: expected value {"abc":123} for StringExact check, got: {"abc":123}

Clearly the values normalize to the same thing, but there's something about the value that comes out wrong and the check fails.

The expected type is definitely a string:

fmt.Fprintf(os.Stderr, "Expected type: %T\n", strings.TrimSpace(tc.Expected))

Expected type: string

I just don't know how to check the expected type being compared with statecheck.ExpectKnownOutputValue().

[SBGoods]

Hi @skyzyx 👋🏾,
Sorry that you're running into to trouble here. The underlying implementation for knownvalue.StringExact() is using a Go string type assertion and making a string comparison. So if you are getting an unexpected error here, I suspect that there might be an issue instead with the way the JSON value is being stored in Terraform state and parsed back into a Go string before the StringExact check is being made. Maybe the error message is obfuscating some string normalization and we can output a more precise value here.

Can you open a new issue with more information about your toml_to_json function implementation?

Thanks