Copilot commented on code in PR #836: URL: https://github.com/apache/solr-operator/pull/836#discussion_r3394380918
########## controllers/events_test.go: ########## @@ -0,0 +1,298 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package controllers + +import ( + "context" + "errors" + "strings" + "testing" + + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + "github.com/go-logr/logr" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +// errForcedPatchFailure is returned by the fake client to force the status patch to fail, +// so that the "could not patch readiness condition" event path is exercised. +var errForcedPatchFailure = errors.New("forced patch failure") + +// requireEvent asserts that exactly one event was recorded whose type and reason match. +// record.FakeRecorder formats each event as "<eventtype> <reason> <message>". Review Comment: The `requireEvent` docstring says it asserts *exactly one* matching event was recorded, but the implementation only asserts that at least one matching event is present (it reads a single event and does not check for additional buffered events). Update the comment to match the behavior, or add an explicit check that the channel is empty afterward. ########## tests/e2e/resource_utils_test.go: ########## @@ -53,6 +53,25 @@ func resourceKey(parentResource client.Object, name string) types.NamespacedName return types.NamespacedName{Name: name, Namespace: parentResource.GetNamespace()} } +// expectEvent waits until at least one Kubernetes Event of the given type and reason has been +// recorded against the given object. Events are matched on the involved object's name within the +// object's namespace, which is unique enough since each event reason is specific to one resource. +func expectEvent(ctx context.Context, parentResource client.Object, eventType string, reason string, additionalOffset ...int) { + EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { + eventList := &corev1.EventList{} + g.Expect(k8sClient.List(ctx, eventList, client.InNamespace(parentResource.GetNamespace()))).To(Succeed()) + matched := false + for i := range eventList.Items { + e := &eventList.Items[i] + if e.InvolvedObject.Name == parentResource.GetName() && e.Type == eventType && e.Reason == reason { + matched = true + break + } + } + g.Expect(matched).To(BeTrue(), "Expected a %q event with reason %q to be recorded on %s/%s", eventType, reason, parentResource.GetNamespace(), parentResource.GetName()) + }).Should(Succeed()) +} Review Comment: The e2e `expectEvent` helper matches events only by involved object name within the namespace. In this test suite the SolrCloud name is always "foo" (see `generateBaseSolrCloud`), and Kubernetes Events are not garbage-collected with the object, so events from prior specs can satisfy the assertion and cause false positives/flaky behavior. Match on `InvolvedObject.UID` (and optionally Kind) instead, like the controller envtest helper does. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
