Allow custom assertions for stashed values

[mamachanko]

Custom assertions for stashed values are helpful when they require bespoke verification. For example:

	rtesting.SubReconcilerTestSuite{
		{
			// ...
			ExpectStashedValues: map[reconcilers.StashKey]interface{}{
				// ...
			},
			VerifyStashedValue: func(t *testing.T, key reconcilers.StashKey, expected, actual interface{}) {
				if diff := cmp.Diff(expected, actual, myCustomOpts); diff != "" {
					t.Errorf("Unexpected stash value %q (-expected, +actual): %s", key, diff)
				}
			},
		},
		// ...
	}   

[codecov]

Codecov Report

Base: 61.64% // Head: 61.55% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (1895c54) compared to base (4768c21).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head 1895c54 differs from pull request most recent head e8bc6f8. Consider uploading reports for the commit e8bc6f8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   61.64%   61.55%   -0.09%     
==========================================
  Files          17       17              
  Lines        2151     2154       +3     
==========================================
  Hits         1326     1326              
- Misses        748      751       +3     
  Partials       77       77              

Impacted Files	Coverage Δ
testing/reconciler.go	0.00% <ø> (ø)
testing/subreconciler.go	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[scothis]

If the goal is to offer alternative validation for stashed values, I think we should offer a hook that is explicitly for validating stashed values.

Something like:

VerifyStashedValue: func(t *testing.T, key reconcilers.StashKey, expected, observed interface{}) {
	if diff := cmp.Diff(expected, actual, ...); diff != "" {
		t.Errorf("Unexpected stash value %q (-expected, +actual): %s", key, diff)
	}
},

We can use the default function if a user doesn't specify an override.

[scothis]

this will allow for additional validation, but it won't suppress existing validation for stashed values.

[mamachanko]

The two might co-exist since

// ExpectStashedValues ensures each value is stashed. Values in the stash that are not expected are ignored. Values in the stash that are not expected are ignored. [...]

And so one might combine the two to achieve the desired coverage.

[mamachanko]

If the goal is to offer alternative validation for stashed values, I think we should offer a hook that is explicitly for validating stashed values.

Something like:

VerifyStashedValue: func(t *testing.T, key reconcilers.StashKey, expected, observed interface{}) {
	if diff := cmp.Diff(expected, actual, ...); diff != "" {
		t.Errorf("Unexpected stash value %q (-expected, +actual): %s", key, diff)
	}
},

That would work for a single entry in the stash. What if a reconciler addresses multiple entries?

We can use the default function if a user doesn't specify an override.

By "default function", do you mean:

• The code in SubReconcilerTestCase which asserts on ExpectStashedValues?
• The existing usage of cmp.Diff(...)?

[scothis]

That would work for a single entry in the stash. What if a reconciler addresses multiple entries?

The current diff is computed for each item in the stash independently.

We can use the default function if a user doesn't specify an override.

By "default function", do you mean:

• The code in SubReconcilerTestCase which asserts on ExpectStashedValues?
• The existing usage of cmp.Diff(...)?

The existing call to cmp.Diff

Something like:

if tc.VerifyStashedValue == nil {
	tc.VerifyStashedValue = func(t *testing.T, key reconcilers.StashKey, expected, observed interface{}) {
		if diff := cmp.Diff(expected, actual, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, IgnoreCreationTimestamp, IgnoreResourceVersion, cmpopts.EquateEmpty()); diff != "" {
			t.Errorf("Unexpected stash value %q (-expected, +actual): %s", key, diff)
		}
	}
}

[mamachanko]

@scothis I get it! You describe a function for verifying each entry listed in ExpectStashedValues. I will incorporate that and update the PR.

[mamachanko]

@scothis your suggestion in incorporated and reflected in the PR's title.

Would you recommend to start testing the (Sub)ReconcilerTests themselves to cover these features? I could start by covering this new addition. Tests for existing testing facilities could/would be backfilled eventually. Yet I wonder if we would ever get around to it.

[scothis]

looking good, a couple small tweeks inline

[mamachanko]

looking good, a couple small tweeks inline

@scothis Incorporated your suggestions. Thanks for reminding me of tc.Prepare!