VC-35738: Assert the logs in TestNoneCache

[maelvls]

In #612, it is written:

// TestNoneCache demonstrates that the cache helpers do not crash if passed a
// non-cachable object, but log an error with a reference to the object type.

It says that "an error is logged", but the logs aren't asserted. I found ktesting's documentation (https://pkg.go.dev/k8s.io/klog/v2/ktesting) that it is possible to assert the logs. I've tried doing that in this PR.

WDYT?

[wallrj]

I've stated my case below. Interested to hear your counter arguments.

[wallrj]

In config_test I used ktesting like this:

jetstack-secure/pkg/agent/config_test.go

Lines 998 to 1003 in 1a567ef

	func recordLogs(t *testing.T) (logr.Logger, ktesting.Buffer) {
	log := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true)))
	testingLogger, ok := log.GetSink().(ktesting.Underlier)
	require.True(t, ok)
	return log, testingLogger.GetBuffer()
	}

// TestNoneCache demonstrates that the cache helpers do not crash if passed a
// non-cachable object, but log an error with a reference to the object type.
func TestNoneCache(t *testing.T) {
	log := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.Verbosity(10), ktesting.BufferLogs(true)))
	testingLogger, ok := log.GetSink().(ktesting.Underlier)
	require.True(t, ok)
	buffer := testingLogger.GetBuffer()
	type notCachable struct{}
	onAdd(log, &notCachable{}, nil)
	onUpdate(log, &notCachable{}, nil, nil)
	onDelete(log, &notCachable{}, nil)
	assert.Equal(t, testutil.Undent(`
		ERROR Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="add"
		ERROR Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="update"
		ERROR Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="delete"
        `), buffer.String())
}

The advantage is that you don't have to worry about timestamps in the logs; you only assert that the logged messages have the expected severity and that the message and additional structured fields are visible.

An additional advantage, and the real reason I created this test, was to be able to see and verify that the correct line numbers were being logged.

$ go test ./pkg/datagatherer/k8s/... -run TestNoneCache -v
=== RUN   TestNoneCache
    cache.go:53: E1115 09:54:01.716188] Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="add"
    cache.go:66: E1115 09:54:01.716256] Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="update"
    cache.go:80: E1115 09:54:01.716376] Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="delete"
--- PASS: TestNoneCache (0.00s)
PASS
ok      github.com/jetstack/preflight/pkg/datagatherer/k8s      0.032s

I had forgotten to use the WithHelper modifier in the logging helper function and the go test -v output helped me get that right.

jetstack-secure/pkg/datagatherer/k8s/cache.go

Lines 34 to 40 in 1a567ef

	func logCacheUpdateFailure(log logr.Logger, obj interface{}, operation string) {
	// We use WithCallStackHelper to ensure the correct caller line numbers in the log messages
	helper, log := log.WithCallStackHelper()
	helper()
	err := fmt.Errorf("not a cacheResource type: %T missing metadata/uid field", obj)
	log.Error(err, "Cache update failure", "operation", operation)
	}