Skip to content

chore: refactor e2e test diagnostic #1144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
simonpasquier wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: refactor e2e test diagnostic #1144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions pkg/apis/uiplugin/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,37 @@ type UIPluginType string
const (
// TypeDashboards deploys the Dashboards Dynamic Plugin for OpenShift Console.
TypeDashboards UIPluginType = "Dashboards"
// DistributedTracing deploys the Distributed Tracing Dynamic Plugin for the OpenShift Console

// TypeDistributedTracing deploys the Distributed Tracing Dynamic Plugin for the OpenShift Console
TypeDistributedTracing UIPluginType = "DistributedTracing"
// TroubleshootingPanel deploys the Troubleshooting Panel Dynamic Plugin for the OpenShift Console

// TypeTroubleshootingPanel deploys the Troubleshooting Panel Dynamic Plugin for the OpenShift Console
TypeTroubleshootingPanel UIPluginType = "TroubleshootingPanel"
// Monitoring deploys the Monitoring Plugin for the OpenShift Console

// TypeMonitoring deploys the Monitoring Plugin for the OpenShift Console
TypeMonitoring UIPluginType = "Monitoring"

// TypeLogging deploys the Logging View Plugin for OpenShift Console.
TypeLogging UIPluginType = "Logging"
)

const (
// DashboardsPluginName is the required name for the Dashboards Plugin resource.
DashboardsPluginName = "dashboards"

// DistributedTracingName is the required name for the DistributedTracing Plugin resource.
DistributedTracingName = "distributed-tracing"

// TroubleshootingPanelName is the required name for the TroubleshootingPanel Plugin resource.
TroubleshootingPanelName = "troubleshooting-panel"

// MonitoringName is the required name for the Monitoring Plugin resource.
MonitoringName = "monitoring"

// LoggingName is the required name for the Logging Plugin resource.
LoggingName = "logging"
)

// DeploymentConfig contains options allowing the customization of the deployment hosting the UI Plugin.
type DeploymentConfig struct {
// Define a label-selector for nodes which the Pods should be scheduled on.
Expand Down
44 changes: 21 additions & 23 deletions test/e2e/framework/framework.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,20 +280,20 @@ func (f *Framework) DumpOnFailure(t *testing.T, fns ...DebugFunc) {
if !t.Failed() {
return
}
t.Logf("# Diagnostics for %s (FAILED)", t.Name())
for _, fn := range fns {
fn(t)
}
})
}

// DebugNamespace returns a DebugFunc that dumps deployments, pods, and events
// DebugNamespaces returns a DebugFunc that dumps deployments, pods, and events
// for the given namespaces.
func (f *Framework) DebugNamespace(namespaces ...string) DebugFunc {
func (f *Framework) DebugNamespaces(namespaces ...string) DebugFunc {
return func(t *testing.T) {
t.Helper()
for _, ns := range namespaces {
t.Logf("--- Dumping debug info for namespace %s ---", ns)
f.DumpNamespaceDebug(t, ns)
f.dumpNamespace(t, ns)
}
}
}
Expand Down Expand Up @@ -335,60 +335,58 @@ func (f *Framework) SkipIfClusterVersionBelow(t *testing.T, minVersion string) {
}
}

// DumpNamespaceDebug logs deployments (with conditions), pods (with container
// statuses), and events for the given namespace. Useful as a t.Cleanup or
// on-failure diagnostic helper.
func (f *Framework) DumpNamespaceDebug(t *testing.T, namespace string) {
func (f *Framework) dumpNamespace(t *testing.T, namespace string) {
t.Helper()
ctx := context.WithoutCancel(t.Context())

t.Log("=== BEGIN DEBUG DUMP ===")
defer t.Log("=== END DEBUG DUMP ===")
t.Logf("=== BEGIN NAMESPACE DEBUG DUMP ===")
t.Logf("* Namespace: %s", namespace)
defer t.Log("=== END NAMESPACE DEBUG DUMP ===")

t.Log("\n## Deployments\n")
var deployments appsv1.DeploymentList
if err := f.K8sClient.List(ctx, &deployments, client.InNamespace(namespace)); err != nil {
t.Logf("Failed to list deployments in %s: %v", namespace, err)
t.Logf("Failed to list deployments: %v", err)
} else {
t.Logf("Deployments in namespace %s: %d", namespace, len(deployments.Items))
for _, d := range deployments.Items {
t.Logf(" Deployment: name=%s replicas=%d readyReplicas=%d availableReplicas=%d",
t.Logf("* Deployment: name=%s replicas=%d readyReplicas=%d availableReplicas=%d",
d.Name, ptr.Deref(d.Spec.Replicas, 0), d.Status.ReadyReplicas, d.Status.AvailableReplicas)
for _, c := range d.Status.Conditions {
t.Logf(" condition: type=%s status=%s reason=%s message=%s",
t.Logf(" * condition: type=%s status=%s reason=%s message=%s",
c.Type, c.Status, c.Reason, c.Message)
}
}
}

t.Log("\n## Pods\n")
var pods corev1.PodList
if err := f.K8sClient.List(ctx, &pods, client.InNamespace(namespace)); err != nil {
t.Logf("Failed to list pods in %s: %v", namespace, err)
t.Logf("Failed to list pods: %v", err)
} else {
t.Logf("Pods in namespace %s: %d", namespace, len(pods.Items))
for _, p := range pods.Items {
t.Logf(" Pod: name=%s phase=%s", p.Name, p.Status.Phase)
t.Logf("* Pod: name=%s phase=%s", p.Name, p.Status.Phase)
for _, cs := range p.Status.ContainerStatuses {
switch {
case cs.State.Running != nil:
t.Logf(" container=%s ready=%v restarts=%d state=Running", cs.Name, cs.Ready, cs.RestartCount)
t.Logf(" * container=%s ready=%v restarts=%d state=Running", cs.Name, cs.Ready, cs.RestartCount)
case cs.State.Waiting != nil:
t.Logf(" container=%s ready=%v restarts=%d state=Waiting reason=%s message=%s",
t.Logf(" * container=%s ready=%v restarts=%d state=Waiting reason=%s message=%s",
cs.Name, cs.Ready, cs.RestartCount, cs.State.Waiting.Reason, cs.State.Waiting.Message)
case cs.State.Terminated != nil:
t.Logf(" container=%s ready=%v restarts=%d state=Terminated reason=%s exitCode=%d",
t.Logf(" * container=%s ready=%v restarts=%d state=Terminated reason=%s exitCode=%d",
cs.Name, cs.Ready, cs.RestartCount, cs.State.Terminated.Reason, cs.State.Terminated.ExitCode)
}
}
}
}

t.Log("\n## Events\n")
var events corev1.EventList
if err := f.K8sClient.List(ctx, &events, client.InNamespace(namespace)); err != nil {
t.Logf("Failed to list events in %s: %v", namespace, err)
t.Logf("Failed to list events: %v", err)
} else {
t.Logf("Events in namespace %s: %d", namespace, len(events.Items))
for _, e := range events.Items {
t.Logf(" Event: involvedObject=%s/%s reason=%s message=%s type=%s count=%d",
t.Logf("* Event: involvedObject=%s/%s reason=%s message=%s type=%s count=%d",
e.InvolvedObject.Kind, e.InvolvedObject.Name, e.Reason, e.Message, e.Type, e.Count)
}
}
Expand Down
53 changes: 53 additions & 0 deletions test/e2e/framework/uiplugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package framework

import (
"context"
"testing"

"sigs.k8s.io/controller-runtime/pkg/client"

uiv1 "github.com/rhobs/observability-operator/pkg/apis/uiplugin/v1alpha1"
)

func (f *Framework) DebugUIPlugin(pluginName string) DebugFunc {
return func(t *testing.T) {
t.Helper()
ctx := context.WithoutCancel(t.Context())

t.Logf("## UIPlugin %s", pluginName)
var plugin uiv1.UIPlugin
if err := f.K8sClient.Get(ctx, client.ObjectKey{Name: pluginName}, &plugin); err != nil {
t.Logf("Failed to get UIPlugin %q: %v", pluginName, err)
return
}
t.Logf("* UIPlugin %q generation=%d, resourceVersion=%s", pluginName, plugin.Generation, plugin.ResourceVersion)
t.Logf(" * UIPlugin spec.type=%s", plugin.Spec.Type)
if plugin.Spec.Monitoring != nil {
if plugin.Spec.Monitoring.ClusterHealthAnalyzer != nil {
t.Logf(" * UIPlugin spec.monitoring.clusterHealthAnalyzer.enabled=%v", plugin.Spec.Monitoring.ClusterHealthAnalyzer.Enabled)
}
if plugin.Spec.Monitoring.Incidents != nil {
t.Logf(" * UIPlugin spec.monitoring.incidents.enabled=%v", plugin.Spec.Monitoring.Incidents.Enabled)
}
}
t.Log("* UIPlugin conditions")
if len(plugin.Status.Conditions) == 0 {
t.Log(" * No status conditions")
}
for _, c := range plugin.Status.Conditions {
t.Logf(" * Condition: type=%s status=%s reason=%s message=%s", c.Type, c.Status, c.Reason, c.Message)
}

t.Logf("## UIPlugins summary")

var plugins uiv1.UIPluginList
if err := f.K8sClient.List(ctx, &plugins); err != nil {
t.Logf("Failed to list UIPlugins: %v", err)
} else {
t.Logf("* Total number of UIPlugins: %d", len(plugins.Items))
for _, p := range plugins.Items {
t.Logf(" * UIPlugin: name=%s type=%s conditions=%d", p.Name, p.Spec.Type, len(p.Status.Conditions))
}
}
}
}
2 changes: 1 addition & 1 deletion test/e2e/monitoring_stack_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func assertCRDExists(t *testing.T, crds ...string) {
}

func TestMonitoringStackController(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(e2eTestNamespace))
f.DumpOnFailure(t, f.DebugNamespaces(e2eTestNamespace))
err := stack.AddToScheme(scheme.Scheme)
assert.NilError(t, err, "adding stack to scheme failed")
assertCRDExists(t,
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/observability_installer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestObservabilityInstallerController(t *testing.T) {
}

func testObservabilityInstallerTracing(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(f.OperatorNamespace, "tracing-observability"))
f.DumpOnFailure(t, f.DebugNamespaces(f.OperatorNamespace, "tracing-observability"))
ctx := context.Background()

// The ObservabilityInstaller installs operators via subscriptions,
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/operator_metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

func TestOperatorMetrics(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(f.OperatorNamespace))
f.DumpOnFailure(t, f.DebugNamespaces(f.OperatorNamespace))
t.Run("operator exposes metrics", func(t *testing.T) {
pod := f.GetOperatorPod(t)

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/po_admission_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
)

func TestPrometheusRuleWebhook(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(e2eTestNamespace))
f.DumpOnFailure(t, f.DebugNamespaces(e2eTestNamespace))
assertCRDExists(t,
"prometheusrules.monitoring.rhobs",
)
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/prometheus_operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type testCase struct {
}

func TestPrometheusOperatorForNonOwnedResources(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(e2eTestNamespace))
f.DumpOnFailure(t, f.DebugNamespaces(e2eTestNamespace))
resources := []client.Object{
newPrometheus(nil),
newAlertmanager(nil),
Expand Down Expand Up @@ -75,7 +75,7 @@ func TestPrometheusOperatorForNonOwnedResources(t *testing.T) {
}

func TestPrometheusOperatorForOwnedResources(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(e2eTestNamespace))
f.DumpOnFailure(t, f.DebugNamespaces(e2eTestNamespace))
resources := []client.Object{
newPrometheus(ownedResourceLabels),
newAlertmanager(ownedResourceLabels),
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/thanos_querier_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
)

func TestThanosQuerierController(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(e2eTestNamespace))
f.DumpOnFailure(t, f.DebugNamespaces(e2eTestNamespace))
assertCRDExists(t, "thanosqueriers.monitoring.rhobs")

ts := []testCase{
Expand Down
47 changes: 4 additions & 43 deletions test/e2e/uiplugin_cluster_health_analyzer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/controller-runtime/pkg/client"

uiv1 "github.com/rhobs/observability-operator/pkg/apis/uiplugin/v1alpha1"
"github.com/rhobs/observability-operator/test/e2e/framework"
Expand All @@ -32,13 +31,13 @@ func clusterHealthAnalyzer(t *testing.T) {
err := monv1.AddToScheme(f.K8sClient.Scheme())
assert.NilError(t, err, "failed to add monv1 to scheme")

f.DumpOnFailure(t, f.DebugNamespace(uiPluginInstallNS))
f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS))

plugin := resetMonitoringUIPlugin(t)
err = f.K8sClient.Create(t.Context(), plugin)
assert.NilError(t, err, "failed to create monitoring UIPlugin")

f.DumpOnFailure(t, dumpUIPluginDebug(plugin.Name))
f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))
Comment on lines +34 to +40

@coderabbitai coderabbitai Bot Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Register all relevant failure diagnostics before the first assertion can fail.

plugin.Name is known before the create, so f.DebugUIPlugin(plugin.Name) should be registered before Line 37; otherwise a create failure skips the new UIPlugin dump entirely. Also, this test later creates the PrometheusRule in prometheusRuleNamespace, but the namespace dump only covers uiPluginInstallNS, so failures in the alert/incident waits miss the rule namespace events and objects.

Suggested fix 
-	f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS))
+	f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS, prometheusRuleNamespace))

 	plugin := resetMonitoringUIPlugin(t)
+	f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))
 	err = f.K8sClient.Create(t.Context(), plugin)
 	assert.NilError(t, err, "failed to create monitoring UIPlugin")
-
-	f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))

As per path instructions, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS))
plugin := resetMonitoringUIPlugin(t)
err = f.K8sClient.Create(t.Context(), plugin)
assert.NilError(t, err, "failed to create monitoring UIPlugin")
f.DumpOnFailure(t, dumpUIPluginDebug(plugin.Name))
f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))
f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS, prometheusRuleNamespace))
plugin := resetMonitoringUIPlugin(t)
f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))
err = f.K8sClient.Create(t.Context(), plugin)
assert.NilError(t, err, "failed to create monitoring UIPlugin")
🤖 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/uiplugin_cluster_health_analyzer_test.go` around lines 34 - 40,
Register the UIPlugin and namespace failure diagnostics before the first
potentially failing assertion in the test so a create failure still captures
them: in the test that calls resetMonitoringUIPlugin and f.K8sClient.Create,
move f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name)) ahead of the
create/assertion, and add failure dumping for prometheusRuleNamespace as well
since the later PrometheusRule setup and wait checks depend on it. Keep the
existing uiPluginInstallNS dump, but ensure both namespaces and the plugin dump
are registered early enough to cover all failure paths in this test.

Source: Path instructions

@simonpasquier simonpasquier Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no need to dump the plugin data if it wasn't created.

t.Log("Waiting for health-analyzer deployment to become ready...")
haDeployment := appsv1.Deployment{}
f.GetResourceWithRetry(t, healthAnalyzerDeploymentName, uiPluginInstallNS, &haDeployment)
Expand Down Expand Up @@ -89,7 +88,7 @@ func clusterHealthAnalyzer(t *testing.T) {
func resetMonitoringUIPlugin(t *testing.T) *uiv1.UIPlugin {
plugin := &uiv1.UIPlugin{
ObjectMeta: metav1.ObjectMeta{
Name: "monitoring",
Name: uiv1.MonitoringName,
},
Spec: uiv1.UIPluginSpec{
Type: uiv1.TypeMonitoring,
Expand Down Expand Up @@ -142,6 +141,7 @@ func createRuleNamespace(t *testing.T, name string) {
if err := f.K8sClient.Create(t.Context(), ns); err != nil && !errors.IsAlreadyExists(err) {
t.Fatalf("failed to create rule namespace %s: %v", name, err)
}
f.DumpOnFailure(t, f.DebugNamespaces(name))
f.CleanUp(t, func() {
ctx := context.WithoutCancel(t.Context())
f.K8sClient.Delete(ctx, ns)
Expand Down Expand Up @@ -176,42 +176,3 @@ func newAlwaysFiringRule(t *testing.T, ruleName, alertName string) *monv1.Promet
})
return rule
}

func dumpUIPluginDebug(pluginName string) framework.DebugFunc {
return func(t *testing.T) {
t.Helper()
ctx := context.WithoutCancel(t.Context())

var plugin uiv1.UIPlugin
if err := f.K8sClient.Get(ctx, client.ObjectKey{Name: pluginName}, &plugin); err != nil {
t.Logf("Failed to get UIPlugin %q: %v", pluginName, err)
return
}
t.Logf("UIPlugin %q generation=%d, resourceVersion=%s", pluginName, plugin.Generation, plugin.ResourceVersion)
t.Logf("UIPlugin spec.type=%s", plugin.Spec.Type)
if plugin.Spec.Monitoring != nil {
if plugin.Spec.Monitoring.ClusterHealthAnalyzer != nil {
t.Logf("UIPlugin spec.monitoring.clusterHealthAnalyzer.enabled=%v", plugin.Spec.Monitoring.ClusterHealthAnalyzer.Enabled)
}
if plugin.Spec.Monitoring.Incidents != nil {
t.Logf("UIPlugin spec.monitoring.incidents.enabled=%v", plugin.Spec.Monitoring.Incidents.Enabled)
}
}
if len(plugin.Status.Conditions) == 0 {
t.Log("UIPlugin has no status conditions")
}
for _, c := range plugin.Status.Conditions {
t.Logf("UIPlugin condition: type=%s status=%s reason=%s message=%s", c.Type, c.Status, c.Reason, c.Message)
}

var plugins uiv1.UIPluginList
if err := f.K8sClient.List(ctx, &plugins); err != nil {
t.Logf("Failed to list UIPlugins: %v", err)
} else {
t.Logf("Total UIPlugins in cluster: %d", len(plugins.Items))
for _, p := range plugins.Items {
t.Logf(" UIPlugin: name=%s type=%s conditions=%d", p.Name, p.Spec.Type, len(p.Status.Conditions))
}
}
}
}
2 changes: 1 addition & 1 deletion test/e2e/uiplugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestUIPlugin(t *testing.T) {
}

func dashboardsUIPlugin(t *testing.T) {
f.DumpOnFailure(t, f.DebugNamespace(uiPluginInstallNS))
f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS))
db := newDashboardsUIPlugin(t)
err := f.K8sClient.Create(context.Background(), db)
assert.NilError(t, err, "failed to create a dashboards UIPlugin")
Expand Down
Loading