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

austinvalle commented 3 months ago

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: https://github.com/hashicorp/terraform-provider-corner/pull/228

bflad commented 3 months ago

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 commented 3 months ago

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

hashicorp / terraform-plugin-testing