Skip to content

Add timing instrumentation to HCP e2e tests #114

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

Merged
openshift-merge-bot merged 1 commit into from
Jun 29, 2026
Merged

Add timing instrumentation to HCP e2e tests #114

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
39 changes: 38 additions & 1 deletion test/e2e-cli/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"net/url"
"os"
"os/exec"
"path/filepath"
"strings"
"time"

Expand All @@ -48,6 +49,29 @@ import (
"github.com/openshift/rosa-regional-platform-api/internal/test/thanos"
)

func recordTiming(phase string) func() {
start := float64(time.Now().UnixNano()) / 1e9
return func() {
end := float64(time.Now().UnixNano()) / 1e9
shared := os.Getenv("SHARED_DIR")
if shared == "" {
return
}
status := "ok"
if CurrentSpecReport().Failed() {
status = "error"
}
Comment on lines +60 to +63

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the changed file around the timing helper and the specs that call Skip.
sed -n '1,120p' test/e2e-cli/cluster_test.go
printf '\n---\n'
sed -n '720,820p' test/e2e-cli/cluster_test.go

printf '\n=== search for Skip and timing helper ===\n'
rg -n 'recordTiming|Skip\(' test/e2e-cli/cluster_test.go

Repository: openshift-online/rosa-hyperfleet-api

Length of output: 9517

Skipped cleanup specs need a skipped status

recordTiming() treats every non-failure as "ok", so specs that call Skip(...) after registering the defer still write successful rows to timing.jsonl. Emit a distinct "skipped" status or suppress those rows.

🤖 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-cli/cluster_test.go` around lines 60 - 63, The current status
handling in recordTiming() only distinguishes failures from everything else, so
skipped specs still get recorded as ok. Update the timing status logic in the
cluster_test.go flow around CurrentSpecReport() to detect skipped specs
separately and emit a distinct skipped status, or avoid writing timing rows for
skipped cases entirely. Use the existing recordTiming() and CurrentSpecReport()
paths to locate the change.

record := fmt.Sprintf(`{"phase":%q,"start":%.3f,"end":%.3f,"step":"e2e","status":%q}`,
phase, start, end, status)
f, err := os.OpenFile(filepath.Join(shared, "timing.jsonl"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
return
}
defer f.Close()
fmt.Fprintln(f, record)
Comment on lines +66 to +71

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Don't silently drop timing write failures.

Open/write/close errors are ignored here, so CI can lose timing rows with no signal when ${SHARED_DIR}/timing.jsonl is unwritable or truncated.

Suggested fix 
 		f, err := os.OpenFile(filepath.Join(shared, "timing.jsonl"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
 		if err != nil {
+			GinkgoWriter.Printf("WARNING: failed to open timing log: %v\n", err)
 			return
 		}
-		defer f.Close()
-		fmt.Fprintln(f, record)
+		defer func() {
+			if err := f.Close(); err != nil {
+				GinkgoWriter.Printf("WARNING: failed to close timing log: %v\n", err)
+			}
+		}()
+		if _, err := fmt.Fprintln(f, record); err != nil {
+			GinkgoWriter.Printf("WARNING: failed to write timing log: %v\n", err)
+		}
 	}
 }

As per path instructions, **/*.go: "Never ignore error returns."

📝 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, err := os.OpenFile(filepath.Join(shared, "timing.jsonl"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
return
}
defer f.Close()
fmt.Fprintln(f, record)
f, err := os.OpenFile(filepath.Join(shared, "timing.jsonl"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
GinkgoWriter.Printf("WARNING: failed to open timing log: %v\n", err)
return
}
defer func() {
if err := f.Close(); err != nil {
GinkgoWriter.Printf("WARNING: failed to close timing log: %v\n", err)
}
}()
if _, err := fmt.Fprintln(f, record); err != nil {
GinkgoWriter.Printf("WARNING: failed to write timing log: %v\n", err)
}
🧰 Tools
🪛 golangci-lint (2.12.2)

[error] 70-70: Error return value of f.Close is not checked

(errcheck)

[error] 71-71: Error return value of fmt.Fprintln is not checked

(errcheck)

🤖 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-cli/cluster_test.go` around lines 66 - 71, The timing.jsonl write
path is ignoring open/write/close failures, so CI can lose rows without any
signal. Update the timing write block in the e2e cluster test to check and
handle the error from os.OpenFile, the result of fmt.Fprintln, and the deferred
file close instead of returning silently. Use the surrounding timing-record
helper in cluster_test.go to surface these failures so unwritable or truncated
SHARED_DIR writes are reported.

Sources: Path instructions, Linters/SAST tools

}
}

func customerEnv() []string {
return []string{"AWS_PROFILE=" + os.Getenv("CUSTOMER_AWS_PROFILE")}
}
Expand Down Expand Up @@ -304,7 +328,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {

// create a new cluster-vpc
It("should be able to create a new cluster-vpc", Label("vpc-create", "setup"), func() {
// wait for the command to complete, it will take a few minutes.
defer recordTiming("hcp-vpc-create")()
GinkgoWriter.Printf("Creating new cluster-vpc: %s\n", clusterName)
// GinkgoWriter.Printf("Command: %s %s %s %s %s\n", ROSACTL_BIN, "cluster-vpc", "create", clusterName, "--region", region, "--availability-zones", "us-east-1a")
cmd := exec.Command(ROSACTL_BIN, "cluster-vpc", "create", clusterName, "--region", region, "--availability-zones", "us-east-1a")
Expand Down Expand Up @@ -334,6 +358,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {

// create a new cluster-iam
It("should be able to create the cluster-iam", Label("iam-create", "setup"), func() {
defer recordTiming("hcp-iam-create")()
GinkgoWriter.Printf("Creating new cluster-iam: %s\n", clusterName)
cmd := exec.Command(ROSACTL_BIN, "cluster-iam", "create", clusterName, "--region", region)
cmd.Env = append(os.Environ(), customerEnv()...)
Expand Down Expand Up @@ -382,6 +407,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should be able to create the hcp cluster", Label("hcp-create", "create"), func() {
defer recordTiming("hcp-cluster-create")()
GinkgoWriter.Printf("Creating new HCP cluster: %s\n", clusterName)
cmd := exec.Command(ROSACTL_BIN, "cluster", "create", clusterName, "--region", region, "--output", "json")
cmd.Env = append(os.Environ(), customerEnv()...)
Expand Down Expand Up @@ -455,6 +481,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should be able to create the cluster-oidc", Label("oidc-create", "setup"), func() {
defer recordTiming("hcp-oidc-create")()
GinkgoWriter.Printf("Creating new cluster-oidc: %s\n", clusterName)
if cloudUrl == "" {
cloudUrl = os.Getenv("HCP_ROSA_ISSUER_URL")
Expand Down Expand Up @@ -488,6 +515,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
// GET /api/v0/clusters/{id} and /statuses use the Hyperfleet resource id (e.g. "2pdl6eud5btdtvgv2f4roaca96e9mvtn"),
// not the cluster display name. List responses are { "items": [ { "id", "name", "spec", "status", ... } ], ... }.
It("should be able to wait for the hcp cluster to be ready", Label("cluster-status", "monitor"), func() {
defer recordTiming("hcp-cluster-ready-wait")()
id := clusterID
if id == "" {
id = os.Getenv("HCP_INSTANCE_ID")
Expand Down Expand Up @@ -568,6 +596,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should have valid DNS and TLS for the KAS endpoint", Label("dns-verify", "monitor"), func() {
defer recordTiming("hcp-dns-tls-verify")()
id := clusterID
if id == "" {
id = os.Getenv("HCP_INSTANCE_ID")
Expand Down Expand Up @@ -625,6 +654,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should have nodepools ready", Label("nodepools-wait", "monitor"), func() {
defer recordTiming("hcp-nodepools-wait")()
id := clusterID
if id == "" {
id = os.Getenv("HCP_INSTANCE_ID")
Expand Down Expand Up @@ -705,6 +735,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should be able to delete the hcp cluster", Label("hcp-delete", "cleanup"), func() {
defer recordTiming("hcp-cluster-delete")()
if clusterID == "" {
clusterID = os.Getenv("HCP_INSTANCE_ID")
if clusterID == "" {
Expand All @@ -720,6 +751,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {

// it should be able to query the /cluster/id until it is deleted
It("should be able to query the /cluster/id until it is deleted", Label("hcp-delete", "cluster-query", "cleanup"), func() {
defer recordTiming("hcp-cluster-delete-wait")()
GinkgoWriter.Printf("Querying the hcp clusterId: %s\n", clusterID)
if clusterID == "" {
clusterID = os.Getenv("HCP_INSTANCE_ID")
Expand All @@ -736,6 +768,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should be able to delete the resource bundles", Label("hcp-delete", "bundles-delete", "cleanup"), func() {
defer recordTiming("hcp-bundles-delete")()
if clusterID == "" {
clusterID = os.Getenv("HCP_INSTANCE_ID")
if clusterID == "" {
Expand All @@ -748,6 +781,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should wait for resource bundles to be fully removed", Label("bundles-wait", "cleanup"), func() {
defer recordTiming("hcp-bundles-wait")()
if clusterID == "" {
clusterID = os.Getenv("HCP_INSTANCE_ID")
if clusterID == "" {
Expand All @@ -767,6 +801,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should be able to delete the cluster-oidc", Label("oidc-delete", "cleanup"), func() {
defer recordTiming("hcp-oidc-delete")()
GinkgoWriter.Printf("Deleting the cluster-oidc: %s\n", clusterName)
cmd := exec.Command(ROSACTL_BIN, "cluster-oidc", "delete", clusterName, "--region", region)
cmd.Env = append(os.Environ(), customerEnv()...)
Expand All @@ -779,6 +814,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {

// Delete cluster-vpc with up to 3 attempts; fail the spec if all attempts return an error.
It("should be able to try to delete the cluster-vpc, trying 3 times", Label("vpc-delete", "cleanup"), func() {
defer recordTiming("hcp-vpc-delete")()
const maxAttempts = 3
const backoffBetweenAttempts = 5 * time.Minute

Expand Down Expand Up @@ -818,6 +854,7 @@ var _ = Describe("ROSACTL CLI E2E Tests", Ordered, func() {
})

It("should be able to delete the cluster-iam", Label("iam-delete", "cleanup"), func() {
defer recordTiming("hcp-iam-delete")()
GinkgoWriter.Printf("Deleting the cluster-iam: %s\n", clusterName)
cmd := exec.Command(ROSACTL_BIN, "cluster-iam", "delete", clusterName, "--region", region)
cmd.Env = append(os.Environ(), customerEnv()...)
Expand Down