test/e2e/apiextensions_compositions_test.go (2)

542-562: ⚡ Quick win

Same t.Errorf → t.Fatalf concern applies here; also, the modify patch drops the app: crossplane-e2e label.

Two small points:

1. t.Errorf + return ctx (line 556) shares the same concern as the setup step — downstream assertions become noisy. t.Fatalf is cleaner.
2. The creation patch (line 517) sets labels: {app: crossplane-e2e}. The modify patch here omits labels entirely. Because both patches are under the same FieldOwner, SSA will relinquish ownership of the app label and it will be removed. This is harmless for the test's purpose, but worth knowing — could you confirm this is intentional?

🛠️ Proposed fix for t.Fatalf

-			if err := cfg.Client().Resources().GetControllerRuntimeClient().Patch(ctx, cm, client.Apply, client.FieldOwner(FieldManager), client.ForceOwnership); err != nil {
-				t.Errorf("failed to modify ConfigMap: %v", err)
-				return ctx
-			}
+			if err := cfg.Client().Resources().GetControllerRuntimeClient().Patch(ctx, cm, client.Apply, client.FieldOwner(FieldManager), client.ForceOwnership); err != nil {
+				t.Fatalf("failed to modify ConfigMap: %v", err)
+			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/apiextensions_compositions_test.go` around lines 542 - 562, In the
Assess "ModifyConfigMapDirectly" closure, replace the t.Errorf call that returns
ctx with t.Fatalf to stop the test on patch failure (change the error path in
the closure using t.Fatalf). Also ensure the patch preserves the app label so
SSA doesn't remove it: when building the unstructured cm before calling
cfg.Client().Resources().GetControllerRuntimeClient().Patch(..., client.Apply,
client.FieldOwner(FieldManager), ...), set metadata.labels.app="crossplane-e2e"
(or fetch the existing resource first and modify only data.message) so the
FieldManager/Apply call does not drop the label; confirm if removing the label
was intentional and keep the label-preserving approach if not.

---

509-528: ⚡ Quick win

Consider using t.Fatalf instead of t.Errorf in the setup step.

t.Errorf marks the test failed but execution continues, so all downstream Assess steps will also run (and likely fail with confusing errors) even though the ConfigMap was never created. t.Fatalf immediately stops the goroutine and gives a cleaner failure signal. The existing ModifyConfigMapDirectly step at line 556 has the same pattern.

Contrast this with TestCircuitBreaker's inline step (line 387), which uses t.Logf because failures there are non-critical; here the ConfigMap must exist for the test to make sense.

🛠️ Proposed fix

-			if err := cfg.Client().Resources().GetControllerRuntimeClient().Patch(ctx, cm, client.Apply, client.FieldOwner(FieldManager), client.ForceOwnership); err != nil {
-				t.Errorf("failed to create ConfigMap: %v", err)
-				return ctx
-			}
+			if err := cfg.Client().Resources().GetControllerRuntimeClient().Patch(ctx, cm, client.Apply, client.FieldOwner(FieldManager), client.ForceOwnership); err != nil {
+				t.Fatalf("failed to create ConfigMap: %v", err)
+			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/apiextensions_compositions_test.go` around lines 509 - 528, Replace
the non-fatal test failure in the setup step by calling t.Fatalf instead of
t.Errorf inside the WithSetup("CreateConfigMap", func(ctx context.Context, t
*testing.T, cfg *envconf.Config) ...) so the test goroutine stops immediately
when the ConfigMap creation Patch fails; do the same change for the similar
setup helper used by ModifyConfigMapDirectly so that creation failures abort the
test rather than allowing downstream Assess steps to run.

Fix all unresolved CodeRabbit comments on this PR:

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