Closed neowulf closed 4 months ago
dprotaso
commented
4 months ago when using cmp package with protobuf you'll need to use protocmp.Transform() so it doesn't error out on these unexported fields.
https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp
Unsure how to wire that option into reconciler-runtime testing
neowulf
commented
4 months ago I have the following snippet in my reconciler code which works when the comparator used in the code is from the k8s.io/apimachinery:
func init() {
if err := equality.Semantic.AddFunc(func(dr1, dr2 *v1beta1.DestinationRule) bool {
return cmp.Equal(&dr1.Spec, &dr2.Spec, protocmp.Transform())
}); err != nil {
panic(err)
}
}As @dprotaso points out - I also don't see how I can add this comparator when the lib is directly using github.com/google/go-cmp.
Can the library be updated to provide custom comparators through init function?
neowulf
commented
4 months ago I am thinking we can add a property called Comparators in the testing and reconcilers package:
Comparators = make([]cmp.Option, 0)And update the existing cmp.Diff functions to do one of the following:
cmp.Diff(exp, actual, Comparators...)
// or when existing comparators exist:
cmp.Diff(exp, actual, append(Comparators, NormalizeLabelSelector, NormalizeFieldSelector)...)The following would be the usage in the test file:
func init() {
rtesting.Comparators = append(rtesting.Comparators, protocmp.Transform())
}
mamachanko
commented
4 months ago @neowulf when you say (emphasis mine)
I have the following snippet in my reconciler code which works when the comparator used in the code is from the
k8s.io/apimachinery:func init() { if err := equality.Semantic.AddFunc(func(dr1, dr2 *v1beta1.DestinationRule) bool { return cmp.Equal(&dr1.Spec, &dr2.Spec, protocmp.Transform()) }); err != nil { panic(err) } }
do you mean that your tests perform as expected or your reconciler works as expected? I assume the latter but want to double-check.
As for a solution, I appreciate your idea concerning comparators. However, I am thinking more of something similar to VerifyStashedValue (See https://github.com/vmware-labs/reconciler-runtime/pull/311, testing#VerifyStashedValueFunc and readme#stash). Instead of exposing the underlying assertion machinery, i.e. cmp, VerifyStashedValueFunc provides an escape hatch for a fully-customizable assertion function. It can use cmp but is not limited to it.
We have a reconciler that stashes x509.Certificate and this type has un-exported fields as well. We are providing this custom func to VerifyStashedValue:
func verifyStashedValue(t *testing.T, key reconcilers.StashKey, expected, actual interface{}) {
if diff := cmp.Diff(expected, actual, cmp.AllowUnexported(big.Int{})); diff != "" {
t.Errorf("Unexpected stash value %q (-expected, +actual): %s", key, diff)
}
}A similar approach could be extended to Expect*. The challenge is finding the right level of assertion. While there's one stash to assert against, there's an array of objects/statuses that are CRUD'd. 🤔
The naive
type VerifyExpectCreatesFunc func(t *testing.T, expected, actual []client.Object)
// or
type VerifyExpectCreatesFunc func(expected, actual []client.Object) errormight require the user to be very diligent in reimplementing what https://github.com/vmware-labs/reconciler-runtime/blob/main/testing/config.go#L369-L391 does for them. But it could work.
This should then be replicated for all the CRUD ops. Unless there's a simpler way.
I might be able to draft this for you, but it might take a moment.
mamachanko
commented
4 months ago type VerifyExpectCreatesFunc func(expected, actual client.Object) errormight be simpler. reconciler-runtime would continue to assert the cardinality of expected and actual creates. It would call the user-provided VerifyExpectCreates with the actual and expected objects.
neowulf
commented
4 months ago Sorry for the confusion. Yes, that snippet is for the reconcilers to run. This bug is regarding the tests.
My test setup is failing when I define the expected as part of the ExpectCreates as part of the ReconcilerTests.
scothis
commented
4 months ago Should be resolved by #487
Describe the bug
reconciler-runtime: v0.18.0
Panic occurs when Istio types are used in the Reconciler tests.
Reproduction steps
Provide an Istio data type in the
ExpectCreates"Expected behavior
Panic should not occur.
Additional context
Open to alternative ways of normalizing the istio data types to something the library can understand.
Passing in the unstructured struct doesn't seem to help because of the differing data types:
Background
Istio datatypes internally wrap protobuf messages in their k8s datatypes: