internal/controller/protection/usage/reconciler_test.go (1)

281-349: Could we add one companion case for len(usages) > 0?

Nice coverage here. To fully protect this branch’s intent, would you add a case where FindUsageOf returns at least one usage and assert the crossplane.io/in-use label is not removed while the namespaced Usage is still rejected? That would prevent regressions in the “remove only when no valid usages require it” behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/protection/usage/reconciler_test.go` around lines 281 -
349, Add a new test case mirroring
"RejectNamespacedUsageOfClusterScopedResource" but have FinderFn return a
non-empty slice (e.g., []protection.Usage{&protection.InternalUsage{}}) so
len(usages) > 0; keep the same WithClientApplicator setup but change MockUpdate
(for composed.Unstructured) to assert the inUseLabelKey is still present (fail
if it's missing) and keep MockStatusUpdate asserting the Usage is rejected with
the same ReconcileError; ensure the test uses the same Reconciler options
(WithSelectorResolver, WithFinalizer) and expects the same err value as the
original case.

internal/webhook/protection/usage/validating_test.go (1)

345-354: Test execution looks good!

The test runner follows the standard table-driven pattern. One small thought: the DenyBadObject test case (lines 89-91) compares the exact error message text including "invalid character 'o' in literal null (expecting 'u')". This could potentially be fragile if the JSON library's error messages change across Go versions, though in practice this tends to be stable. Just something to be aware of if this test ever becomes flaky!

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/webhook/protection/usage/validating_test.go` around lines 345 - 354,
The test is fragile because the DenyBadObject case asserts an exact JSON parser
error string; update the assertion in the table-driven test that constructs
NewValidatingHandler and calls Handle so it checks for the presence of a stable
substring or error type instead of matching the full message (e.g., use
strings.Contains on got.Error() or compare against a sentinel error) to make the
DenyBadObject expectation resilient to Go/JSON error text changes.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/controller/protection/usage/reconciler_test.go (2)

319-333: Consider using cmpopts.IgnoreFields or consolidating the condition verification.

Both test cases have identical status verification logic in their MockStatusUpdate callbacks. This duplication could be extracted into a helper function for clarity. However, given that this follows the existing patterns in the file (e.g., similar verbose checks at lines 545-548), I understand if you prefer to keep it consistent with the established style.

This is a minor suggestion—the current implementation is correct and readable.

♻️ Optional: Extract status verification helper

func verifyReconcileErrorCondition(t *testing.T, o *v1beta1.Usage, wantErr error) {
	t.Helper()
	got := o.Status.GetCondition(xpv2.TypeSynced)
	want := xpv2.ReconcileError(wantErr)
	if diff := cmp.Diff(want.Status, got.Status); diff != "" {
		t.Fatalf("unexpected synced status: %s", diff)
	}
	if diff := cmp.Diff(want.Reason, got.Reason); diff != "" {
		t.Fatalf("unexpected synced reason: %s", diff)
	}
	if diff := cmp.Diff(want.Message, got.Message); diff != "" {
		t.Fatalf("unexpected synced message: %s", diff)
	}
}

Then in MockStatusUpdate:

MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil, func(obj client.Object) error {
	verifyReconcileErrorCondition(t, obj.(*v1beta1.Usage), 
		errors.Errorf("%s: Usage %q in namespace %q references cluster-scoped %s %q; use ClusterUsage instead", 
			errUnsupportedUsage, "protect-cluster", "default", "ClusterThing", "cluster-resource"))
	return nil
}),

Also applies to: 386-400

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/protection/usage/reconciler_test.go` around lines 319 -
333, Tests duplicate the detailed condition verification in multiple
MockStatusUpdate callbacks; extract that logic into a helper and call it from
each callback to remove duplication. Add a helper function (e.g.,
verifyReconcileErrorCondition(t *testing.T, o *v1beta1.Usage, wantErr error))
that calls o.Status.GetCondition(xpv2.TypeSynced), builds the expected
xpv2.ReconcileError(wantErr) and runs the three cmp.Diff checks, then replace
the inline checks inside test.NewMockSubResourceUpdateFn callbacks (the
MockStatusUpdate blocks) to call this helper with the constructed errors (using
errUnsupportedUsage and the same Usage name/namespace/cluster resource strings).
This keeps behavior identical while removing duplicated code.

---

281-349: Resolve MockUpdate inconsistency between the two test cases.

The annotation update logic at lines 449-455 in the reconciler is conditional—it only calls client.Update() if the computed annotation value differs from the current one. Since both test cases pre-set the annotation to "undefined", the reconciler will attempt to update it with the computed value. When the Update call fails (as mocked in the first test), the error is logged but doesn't stop execution, allowing the reconciliation to continue and reach the cluster-scope check.

However, there's an inconsistency: the first test's MockUpdate returns an error for non-Unstructured objects (lines 309-317), while the second test returns nil (lines 378-385). Both tests reach the same endpoint (setting the ReconcileError condition), so clarify why this differs between them. Either both should return an error to match the actual behavior, or both should return nil if the annotation update is expected to succeed. This will improve test clarity and ensure the mocking is intentional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/protection/usage/reconciler_test.go` around lines 281 -
349, The MockUpdate behavior in the two related tests is inconsistent: the first
test's test.MockClient.MockUpdate returns an error for
non-*composed.Unstructured objects while the second returns nil, but both
exercises hit the reconciler's annotation-update path (which only calls
client.Update when the computed annotation differs) and then proceed to the same
cluster-scope rejection logic; make them consistent by updating the first test's
MockUpdate in the RejectNamespacedUsageOfClusterScopedResource case (or the
second test) so both MockUpdate handlers either return nil for the expected
object types or both simulate the same update error, referencing the
test.MockClient.MockUpdate, WithClientApplicator/xpresource.ClientApplicator,
and detailsAnnotationKey/*composed.Unstructured handling to locate the mocks to
change.