Filter tests are fragile and hard to understand

[johananl]

While working on a change in the bootstrap API I've encountered tests such as the following:

cluster-api/controlplane/kubeadm/internal/filters_test.go

Lines 535 to 554 in 010af7f

	g.Expect(diff).To(BeComparableTo(`&v1beta1.KubeadmConfigSpec{
	ClusterConfiguration: nil,
	InitConfiguration: &v1beta1.InitConfiguration{
	TypeMeta: {},
	BootstrapTokens: nil,
	NodeRegistration: v1beta1.NodeRegistrationOptions{
	- Name: "",
	+ Name: "A new name",
	CRISocket: "",
	Taints: nil,
	... // 4 identical fields
	},
	LocalAPIEndpoint: {},
	SkipPhases: nil,
	Patches: nil,
	},
	JoinConfiguration: nil,
	Files: nil,
	... // 10 identical fields
	}`))

When such tests failed for me I found it very tricky to figure out what's wrong. This is because the tests check the correctness of a diff value that's returned as a string from the function under test and prints any mismatches to the developer as a diff of two diffs (returned string vs expected string). Here is a sample failure:

--- FAIL: TestMatchInitOrJoinConfiguration (0.00s)
    --- FAIL: TestMatchInitOrJoinConfiguration/returns_false_if_InitConfiguration_is_NOT_equal (0.00s)
        filters_test.go:587:
            Expected object to be comparable, diff:   (
                """
                ... // 17 identical lines
                      JoinConfiguration:  nil,
                      PreKubeadmCommands: nil,
            -         ... // 2 identical fields
                    },
                    Files:     nil,
                    DiskSetup: nil,
            -       ... // 6 identical fields
            +       ... // 9 identical fields
                  }
                """
              )
FAIL

When the expected diff string contains a difference (namely lines with +/-), the output gets even more confusing since we now have two "layers" of diffs -- the diff representation in the two diff strings (expected and actual) and the diff between the two diff strings.

In addition, the tests are sensitive to whitespace differences because we rely on string comparison rather than semantic equivalence of two data structures.

I assume there was a good reason for writing these tests like this in the first place. However, I wonder if we can simplify these, get rid of relying on comparing strings somehow or find another way to make these tests less fragile and easier to debug.

cc @sbueringer

[k8s-ci-robot]

This issue is currently awaiting triage.

If CAPI contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[johananl]

/kind cleanup

[sbueringer]

What would you suggest?

[johananl]

I don't have a concrete suggestion. As I wrote above I assume these tests have been written like this because there was no easier/simpler solution.

I think we can't compare structs in the tests because the diff string is the object which the function under test must return. Right?

[johananl]

I wonder if it's important to test the diff strings at all. Of course, we aren't unit testing the functionality of go-cmp here. Rather, we want to ensure functions such as matchInitOrJoinConfiguration() are implemented correctly. I guess a relevant question is if the implementation might break a part of a diff. IOW, maybe it's enough to test for diff empty vs. not empty?

[sbueringer]

These tests are verifying that the diff looks as expected and especially that we notice if anything changes in a way that affects these diffs, basically like a golden file (e.g. we add a new field to the KubeadmConfig CRD that would trigger rollouts after CAPI upgrade)

[johananl]

Yes. I realize that testing the diff contents covers more potential problems than just the existence of a diff. The problem is that IMO the current state is very fragile.

By the way, the go-cmp docs say the following:

Do not depend on this output being stable. If you need the ability to programmatically interpret the difference, consider using a custom Reporter.

I even vaguely remember the library randomizing tabs vs. spaces to actively prevent people from relying on stability there (not sure if that's still the case though).

[sbueringer]

I even vaguely remember the library randomizing tabs vs. spaces to actively prevent people from relying on stability there (not sure if that's still the case though).

I would expect that our tests would not be stable if this would be the case.

[sbueringer]

In my opinion we don't depend on this output being stable. But we do want to know if anything changes, either because we bump the library or because we add fields to the CRDs

[johananl]

Relevant issue: google/go-cmp#344

I wonder what a custom reporter is. Maybe that's what gomega does and hence the tests don't fail at random.

[johananl]

In any case, I opened this since I had a hard time fixing the broken tests while working on making some KubeadmConfig fields reusable. If you think there is nothing to improve here we can close 👍

[sbueringer]

I don't think gomega injects a custom reporter into the go-cmp library behind our back :)

[johananl]

I don't think gomega injects a custom reporter into the go-cmp library behind our back :)

Yeah, I couldn't find such a thing either. In the past I'm 90% sure I saw go-cmp replace tabs with spaces and vice versa at random to prevent me from programmatically comparing diffs.

[sbueringer]

Maybe BeComparableTo drops whitespace or something (and it uses cmp.Equal underneath)

We definitely use it because the output is better than Equal

[mboersma]

https://pkg.go.dev/github.com/onsi/gomega#BeComparableTo

I was curious about the implementation, so here it is:
https://github.com/onsi/gomega/blob/36fbc8471a1a2391d40b9b8e561e014b3771255c/matchers/be_comparable_to_matcher.go#L16-L40
It basically just calls cmp.Equal after testing for nil and trying a shortcut for byte slices.

[sbueringer]

Okay. I would say let's keep it as is

/close

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

Okay. I would say let's keep it as is

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.