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

internal/lang: ephemeralasnull: Add test for unknown values #35673

Closed
wants to merge 1 commit into from

Conversation

radeksimko
Copy link
Member

This is a small PR following up on #35652 which I wanted to split from #35653 as that PR may require some further discussions.

It covers the extra logic present in the function implementation concerning unknown values:

if !v.IsKnown() {
// If the source value is unknown then we must leave it
// unknown because its final type might be more precise
// than the associated type constraint and returning a
// typed null could therefore over-promise on what the
// final result type will be.
// We're deliberately constructing a fresh unknown value
// here, rather than returning the one we were given,
// because we need to discard any refinements that the
// unknown value might be carrying that definitely won't
// be honored when we force the final result to be null.
return cty.UnknownVal(v.Type()).WithMarks(givenMarks), nil
}

In other words, the test (and underlying logic under test) ensures that other marks (other than ephemerality) are retained while passing through that function.

@apparentlymart
Copy link
Contributor

This seems to be gradually growing to replicate several cases already covered in this function's main tests:

func TestEphemeralAsNull(t *testing.T) {
tests := []struct {
Input cty.Value
Want cty.Value
}{
// Simple cases
{
cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral),
cty.NullVal(cty.String),
},
{
cty.StringVal("hello"),
cty.StringVal("hello"),
},
{
// Unknown values stay unknown because an unknown value with
// an imprecise type constraint is allowed to take on a more
// precise type in later phases, but known values (even if null)
// should not. We do know that the final known result definitely
// won't be ephemeral, though.
cty.UnknownVal(cty.String).Mark(marks.Ephemeral),
cty.UnknownVal(cty.String),
},
{
cty.UnknownVal(cty.String),
cty.UnknownVal(cty.String),
},
{
// Unknown value refinements should be discarded when unmarking,
// both because we know our final value is going to be null
// anyway and because an ephemeral value is not required to
// have consistent refinements between the plan and apply phases.
cty.UnknownVal(cty.String).RefineNotNull().Mark(marks.Ephemeral),
cty.UnknownVal(cty.String),
},
{
// Refinements must be preserved for non-ephemeral values, though.
cty.UnknownVal(cty.String).RefineNotNull(),
cty.UnknownVal(cty.String).RefineNotNull(),
},
// Should preserve other marks in all cases
{
cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral).Mark(marks.Sensitive),
cty.NullVal(cty.String).Mark(marks.Sensitive),
},
{
cty.StringVal("hello").Mark(marks.Sensitive),
cty.StringVal("hello").Mark(marks.Sensitive),
},
{
cty.UnknownVal(cty.String).Mark(marks.Ephemeral).Mark(marks.Sensitive),
cty.UnknownVal(cty.String).Mark(marks.Sensitive),
},
{
cty.UnknownVal(cty.String).Mark(marks.Sensitive),
cty.UnknownVal(cty.String).Mark(marks.Sensitive),
},
{
cty.UnknownVal(cty.String).RefineNotNull().Mark(marks.Ephemeral).Mark(marks.Sensitive),
cty.UnknownVal(cty.String).Mark(marks.Sensitive),
},
{
cty.UnknownVal(cty.String).RefineNotNull().Mark(marks.Sensitive),
cty.UnknownVal(cty.String).RefineNotNull().Mark(marks.Sensitive),
},
// Nested ephemeral values
{
cty.ListVal([]cty.Value{
cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral),
cty.StringVal("hello"),
}),
cty.ListVal([]cty.Value{
cty.NullVal(cty.String),
cty.StringVal("hello"),
}),
},
{
cty.TupleVal([]cty.Value{
cty.True,
cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral),
cty.StringVal("hello"),
}),
cty.TupleVal([]cty.Value{
cty.True,
cty.NullVal(cty.String),
cty.StringVal("hello"),
}),
},
{
// Sets can't actually preserve individual element marks, so
// this gets treated as the entire set being ephemeral.
// (That's true of the input value, despite how it's written here,
// not just the result value; cty.SetVal does the simplification
// itself during the construction of the value.)
cty.SetVal([]cty.Value{
cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral),
cty.StringVal("hello"),
}),
cty.NullVal(cty.Set(cty.String)),
},
{
cty.MapVal(map[string]cty.Value{
"addr": cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral),
"greet": cty.StringVal("hello"),
}),
cty.MapVal(map[string]cty.Value{
"addr": cty.NullVal(cty.String),
"greet": cty.StringVal("hello"),
}),
},
{
cty.ObjectVal(map[string]cty.Value{
"addr": cty.StringVal("127.0.0.1:12654").Mark(marks.Ephemeral),
"greet": cty.StringVal("hello"),
"happy": cty.True,
}),
cty.ObjectVal(map[string]cty.Value{
"addr": cty.NullVal(cty.String),
"greet": cty.StringVal("hello"),
"happy": cty.True,
}),
},
}
for _, test := range tests {
t.Run(test.Input.GoString(), func(t *testing.T) {
got, err := EphemeralAsNull(test.Input)
if err != nil {
// This function is supposed to be infallible
t.Fatal(err)
}
if diff := cmp.Diff(test.Want, got, ctydebug.CmpOptions); diff != "" {
t.Errorf("wrong result\n%s", diff)
}
})
}
}

Typically these function-table-level tests have been used only to test whether a function is correctly registered in the function table so it can be used at all, rather than getting into all the weeds of how the function behaves. No significant harm in adding these additional tests of course, but I point it out in case you've been doing this just because you didn't know the other test cases were there.

(This mostly-redundant set of tests in the lang package came from a few instances where someone had mistakenly added a function in lang/funcs but had either omitted it from the functions table or had specified it incorrectly so that it couldn't actually be reached, so we added an extra layer of testing to try to cover that mistake.)

@radeksimko
Copy link
Member Author

@apparentlymart Good catch! I didn't notice these tests earlier. I should've checked the test cover more thoroughly..

@radeksimko radeksimko closed this Sep 9, 2024
@radeksimko radeksimko deleted the radek/f-ev-ephemeralasnull-unk-test branch September 9, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants