From 0e67edf77f92046755b12abe8052037121a2fc3b Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 3 Sep 2026 10:20:58 +0000 Subject: [PATCH 1/3] [ote] Add OTE discovery verification and regression test Add a local verification path that catches OTE test-discovery failures before they reach CI. This prevents silent breakage like the missing SpecContext parameter (PR #4566 / OCP-68320) from merging. Changes: - hack/verify-ote-discovery.sh: builds wmco-tests-ext, runs callback- signature static checks, then verifies OTE component registration via the 'list components' subcommand - ote/cmd/wmco-tests-ext/discovery_test.go: Go AST-based regression test that scans all OTE e2e files for g.It callbacks using SpecTimeout/NodeTimeout/GracePeriod decorators without a SpecContext or context.Context parameter - Makefile: adds 'verify-ote-discovery' target and 'verify' alias that chains 'all' (lint build unit) with OTE discovery checks Co-Authored-By: Claude Opus 4.6 --- Makefile | 7 ++ hack/verify-ote-discovery.sh | 43 +++++++ ote/cmd/wmco-tests-ext/discovery_test.go | 147 +++++++++++++++++++++++ 3 files changed, 197 insertions(+) create mode 100755 hack/verify-ote-discovery.sh create mode 100644 ote/cmd/wmco-tests-ext/discovery_test.go diff --git a/Makefile b/Makefile index ea304396bd..bbaa8d6ba0 100644 --- a/Makefile +++ b/Makefile @@ -49,6 +49,13 @@ endif .PHONY: all all: lint build unit +.PHONY: verify +verify: all verify-ote-discovery + +.PHONY: verify-ote-discovery +verify-ote-discovery: + hack/verify-ote-discovery.sh + ##@ General # The help target prints out all targets with their descriptions organized diff --git a/hack/verify-ote-discovery.sh b/hack/verify-ote-discovery.sh new file mode 100755 index 0000000000..840ff9b7c8 --- /dev/null +++ b/hack/verify-ote-discovery.sh @@ -0,0 +1,43 @@ +#!/bin/bash +# verify-ote-discovery.sh — build the OTE extension binary and verify that +# Ginkgo test discovery succeeds. Catches signature errors (e.g. missing +# SpecContext parameter) that silently break all OTE test suites. +set -o errexit +set -o nounset +set -o pipefail + +WMCO_ROOT=$(cd "$(dirname "${BASH_SOURCE}")/.." && pwd) +cd "${WMCO_ROOT}" + +echo "==> Running OTE callback-signature static checks..." +(cd ote && GOFLAGS="" GOWORK=off go test -v -run TestSpecTimeoutCallbackSignatures -count=1 ./cmd/wmco-tests-ext/) + +echo "==> Building wmco-tests-ext..." +make build-tests-ext + +BINARY="build/_output/bin/wmco-tests-ext" +if [ ! -x "${BINARY}" ]; then + echo "ERROR: wmco-tests-ext binary not found at ${BINARY}" + exit 1 +fi + +# The k8s test framework requires KUBECONFIG to be set; create a stub +# so the binary can initialize without a real cluster connection. +FAKE_KUBECONFIG=$(mktemp) +trap "rm -f ${FAKE_KUBECONFIG}" EXIT +export KUBECONFIG="${FAKE_KUBECONFIG}" + +echo "==> Verifying OTE component registration (list components)..." +COMPONENTS=$("${BINARY}" list components 2>&1) || { + echo "ERROR: wmco-tests-ext list components failed" + echo "${COMPONENTS}" + exit 1 +} + +if ! echo "${COMPONENTS}" | grep -q "windows-machine-config-operator"; then + echo "ERROR: expected component 'windows-machine-config-operator' not found" + echo "${COMPONENTS}" + exit 1 +fi + +echo "==> OTE discovery verification passed" diff --git a/ote/cmd/wmco-tests-ext/discovery_test.go b/ote/cmd/wmco-tests-ext/discovery_test.go new file mode 100644 index 0000000000..8555235429 --- /dev/null +++ b/ote/cmd/wmco-tests-ext/discovery_test.go @@ -0,0 +1,147 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestSpecTimeoutCallbackSignatures verifies that every g.It / g.Describe +// callback that uses g.SpecTimeout, g.NodeTimeout, or g.GracePeriod +// decorators has a function parameter accepting g.SpecContext or +// context.Context. Without this parameter Ginkgo panics during test +// discovery ("Invalid NodeTimeout SpecTimeout, or GracePeriod") and +// silently drops every spec in the suite. +// +// This is a regression test for the bug fixed in PR #4566 / OCP-68320. +func TestSpecTimeoutCallbackSignatures(t *testing.T) { + // Decorators that require a context-accepting callback. + timeoutDecorators := map[string]bool{ + "SpecTimeout": true, + "NodeTimeout": true, + "GracePeriod": true, + } + + testDir := filepath.Join("..", "..", "test", "e2e") + entries, err := os.ReadDir(testDir) + if err != nil { + t.Fatalf("failed to read OTE test directory %s: %v", testDir, err) + } + + fset := token.NewFileSet() + var violations []string + + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".go") { + continue + } + if strings.HasSuffix(entry.Name(), "_test.go") { + continue + } + + filePath := filepath.Join(testDir, entry.Name()) + f, err := parser.ParseFile(fset, filePath, nil, 0) + if err != nil { + t.Fatalf("failed to parse %s: %v", filePath, err) + } + + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + + // Match g.It(...) calls — the selector g.It + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + if sel.Sel.Name != "It" { + return true + } + + if len(call.Args) < 2 { + return true + } + + // Check whether any argument is a decorator call + // (g.SpecTimeout, g.NodeTimeout, g.GracePeriod). + hasTimeoutDecorator := false + for _, arg := range call.Args { + if isDecoratorCall(arg, timeoutDecorators) { + hasTimeoutDecorator = true + break + } + } + if !hasTimeoutDecorator { + return true + } + + // Find the callback function literal among arguments. + for _, arg := range call.Args { + funcLit, ok := arg.(*ast.FuncLit) + if !ok { + continue + } + if !callbackAcceptsContext(funcLit) { + pos := fset.Position(funcLit.Pos()) + violations = append(violations, pos.String()) + } + } + return true + }) + } + + if len(violations) > 0 { + t.Errorf("found g.It callbacks with SpecTimeout/NodeTimeout/GracePeriod "+ + "decorators that do not accept a SpecContext or context.Context parameter.\n"+ + "Ginkgo requires the callback to accept a context parameter when these "+ + "decorators are used.\nViolations at:\n %s", + strings.Join(violations, "\n ")) + } +} + +// isDecoratorCall checks whether an AST expression is a call to one of the +// known timeout-related Ginkgo decorator functions (e.g. g.SpecTimeout(...)). +func isDecoratorCall(expr ast.Expr, decorators map[string]bool) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + return decorators[sel.Sel.Name] +} + +// callbackAcceptsContext returns true if the function literal has at least +// one parameter whose type name contains "SpecContext" or "Context". +func callbackAcceptsContext(fn *ast.FuncLit) bool { + if fn.Type.Params == nil || len(fn.Type.Params.List) == 0 { + return false + } + for _, param := range fn.Type.Params.List { + typeName := typeNameString(param.Type) + if strings.Contains(typeName, "SpecContext") || strings.Contains(typeName, "Context") { + return true + } + } + return false +} + +// typeNameString returns a simple string representation of a type expression. +func typeNameString(expr ast.Expr) string { + switch t := expr.(type) { + case *ast.Ident: + return t.Name + case *ast.SelectorExpr: + return typeNameString(t.X) + "." + t.Sel.Name + default: + return "" + } +} From 0573a3cd702151adc295714820cfb39ae393217e Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 3 Sep 2026 18:34:11 +0000 Subject: [PATCH 2/3] [ote] Address review findings in discovery check - hack/verify-ote-discovery.sh: use ${BASH_SOURCE[0]} (SC2128) and single-quoted trap for safe EXIT cleanup (SC2064) - discovery_test.go: expand interruptible-node coverage from It-only to all setup nodes (BeforeEach, AfterEach, JustBeforeEach, JustAfterEach, BeforeAll, AfterAll) per the Ginkgo v2 contract; replace substring context matching with exact type resolution using parsed import aliases, accepting only g.SpecContext or context.Context; enforce the contract-required single-parameter count Co-Authored-By: Claude Opus 4.6 --- hack/verify-ote-discovery.sh | 4 +- ote/cmd/wmco-tests-ext/discovery_test.go | 136 +++++++++++++++++------ 2 files changed, 106 insertions(+), 34 deletions(-) diff --git a/hack/verify-ote-discovery.sh b/hack/verify-ote-discovery.sh index 840ff9b7c8..18da4d5c27 100755 --- a/hack/verify-ote-discovery.sh +++ b/hack/verify-ote-discovery.sh @@ -6,7 +6,7 @@ set -o errexit set -o nounset set -o pipefail -WMCO_ROOT=$(cd "$(dirname "${BASH_SOURCE}")/.." && pwd) +WMCO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) cd "${WMCO_ROOT}" echo "==> Running OTE callback-signature static checks..." @@ -24,7 +24,7 @@ fi # The k8s test framework requires KUBECONFIG to be set; create a stub # so the binary can initialize without a real cluster connection. FAKE_KUBECONFIG=$(mktemp) -trap "rm -f ${FAKE_KUBECONFIG}" EXIT +trap 'rm -f -- "$FAKE_KUBECONFIG"' EXIT export KUBECONFIG="${FAKE_KUBECONFIG}" echo "==> Verifying OTE component registration (list components)..." diff --git a/ote/cmd/wmco-tests-ext/discovery_test.go b/ote/cmd/wmco-tests-ext/discovery_test.go index 8555235429..b05b9bf042 100644 --- a/ote/cmd/wmco-tests-ext/discovery_test.go +++ b/ote/cmd/wmco-tests-ext/discovery_test.go @@ -10,12 +10,18 @@ import ( "testing" ) -// TestSpecTimeoutCallbackSignatures verifies that every g.It / g.Describe -// callback that uses g.SpecTimeout, g.NodeTimeout, or g.GracePeriod -// decorators has a function parameter accepting g.SpecContext or -// context.Context. Without this parameter Ginkgo panics during test -// discovery ("Invalid NodeTimeout SpecTimeout, or GracePeriod") and -// silently drops every spec in the suite. +// TestSpecTimeoutCallbackSignatures verifies that every Ginkgo interruptible +// node (It, BeforeEach, AfterEach, JustBeforeEach, JustAfterEach, BeforeAll, +// AfterAll) that uses a SpecTimeout, NodeTimeout, or GracePeriod decorator +// has a callback with exactly one parameter of type g.SpecContext or +// context.Context in the first position. +// +// Without this parameter Ginkgo panics during test discovery +// ("Invalid NodeTimeout SpecTimeout, or GracePeriod") and silently drops +// every spec in the suite. +// +// Container nodes (Describe, Context, When) are excluded — Ginkgo rejects +// timeout decorators on containers at a different validation layer. // // This is a regression test for the bug fixed in PR #4566 / OCP-68320. func TestSpecTimeoutCallbackSignatures(t *testing.T) { @@ -26,6 +32,20 @@ func TestSpecTimeoutCallbackSignatures(t *testing.T) { "GracePeriod": true, } + // All non-container interruptible node types that support timeout + // decorators per the Ginkgo v2 contract (internal/node.go). + // Container nodes (Describe, Context, When) are excluded because + // Ginkgo rejects timeout decorators on them separately. + interruptibleNodes := map[string]bool{ + "It": true, + "BeforeEach": true, + "AfterEach": true, + "JustBeforeEach": true, + "JustAfterEach": true, + "BeforeAll": true, + "AfterAll": true, + } + testDir := filepath.Join("..", "..", "test", "e2e") entries, err := os.ReadDir(testDir) if err != nil { @@ -49,18 +69,24 @@ func TestSpecTimeoutCallbackSignatures(t *testing.T) { t.Fatalf("failed to parse %s: %v", filePath, err) } + // Resolve import aliases so we can match exact types. + // e.g. g "github.com/onsi/ginkgo/v2" → alias "g" + // "context" → alias "context" + ginkgoAlias := resolveImportAlias(f, "github.com/onsi/ginkgo/v2") + contextAlias := resolveImportAlias(f, "context") + ast.Inspect(f, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) if !ok { return true } - // Match g.It(...) calls — the selector g.It + // Match g.It(...), g.BeforeEach(...), etc. sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { return true } - if sel.Sel.Name != "It" { + if !interruptibleNodes[sel.Sel.Name] { return true } @@ -68,8 +94,7 @@ func TestSpecTimeoutCallbackSignatures(t *testing.T) { return true } - // Check whether any argument is a decorator call - // (g.SpecTimeout, g.NodeTimeout, g.GracePeriod). + // Check whether any argument is a timeout decorator. hasTimeoutDecorator := false for _, arg := range call.Args { if isDecoratorCall(arg, timeoutDecorators) { @@ -87,7 +112,7 @@ func TestSpecTimeoutCallbackSignatures(t *testing.T) { if !ok { continue } - if !callbackAcceptsContext(funcLit) { + if !callbackAcceptsContext(funcLit, ginkgoAlias, contextAlias) { pos := fset.Position(funcLit.Pos()) violations = append(violations, pos.String()) } @@ -97,14 +122,37 @@ func TestSpecTimeoutCallbackSignatures(t *testing.T) { } if len(violations) > 0 { - t.Errorf("found g.It callbacks with SpecTimeout/NodeTimeout/GracePeriod "+ - "decorators that do not accept a SpecContext or context.Context parameter.\n"+ - "Ginkgo requires the callback to accept a context parameter when these "+ - "decorators are used.\nViolations at:\n %s", + t.Errorf("found Ginkgo interruptible-node callbacks with "+ + "SpecTimeout/NodeTimeout/GracePeriod decorators that do not "+ + "accept exactly one parameter of type SpecContext or "+ + "context.Context.\nGinkgo requires the callback to accept a "+ + "context parameter when these decorators are used.\n"+ + "Violations at:\n %s", strings.Join(violations, "\n ")) } } +// resolveImportAlias returns the local alias for an import path. +// If the import has an explicit alias (e.g. g "github.com/onsi/ginkgo/v2"), +// it returns the alias. Otherwise it returns the last path element +// (e.g. "context" for "context", "ginkgo" for ".../ginkgo/v2"). +// Returns "" if the import is not found. +func resolveImportAlias(f *ast.File, importPath string) string { + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, `"`) + if path != importPath { + continue + } + if imp.Name != nil { + return imp.Name.Name + } + // No explicit alias — use last path element. + parts := strings.Split(path, "/") + return parts[len(parts)-1] + } + return "" +} + // isDecoratorCall checks whether an AST expression is a call to one of the // known timeout-related Ginkgo decorator functions (e.g. g.SpecTimeout(...)). func isDecoratorCall(expr ast.Expr, decorators map[string]bool) bool { @@ -119,29 +167,53 @@ func isDecoratorCall(expr ast.Expr, decorators map[string]bool) bool { return decorators[sel.Sel.Name] } -// callbackAcceptsContext returns true if the function literal has at least -// one parameter whose type name contains "SpecContext" or "Context". -func callbackAcceptsContext(fn *ast.FuncLit) bool { +// callbackAcceptsContext returns true if the function literal has exactly +// one parameter and that parameter's type is either .SpecContext +// or .Context. This matches the Ginkgo v2 contract which +// requires exactly func() or func(SpecContext)/func(context.Context) — no +// other parameter counts are accepted for interruptible bodies. +func callbackAcceptsContext(fn *ast.FuncLit, ginkgoAlias, contextAlias string) bool { if fn.Type.Params == nil || len(fn.Type.Params.List) == 0 { return false } - for _, param := range fn.Type.Params.List { - typeName := typeNameString(param.Type) - if strings.Contains(typeName, "SpecContext") || strings.Contains(typeName, "Context") { - return true + + // Ginkgo's extractBodyFunction rejects callbacks with >1 parameter + // or any return values. Count total parameters (a field may declare + // multiple names, e.g. "a, b int"). + totalParams := 0 + for _, field := range fn.Type.Params.List { + names := len(field.Names) + if names == 0 { + names = 1 // unnamed parameter } + totalParams += names } - return false + if totalParams != 1 { + return false + } + + // Check the first (and only) parameter type. + firstParam := fn.Type.Params.List[0] + return isExactContextType(firstParam.Type, ginkgoAlias, contextAlias) } -// typeNameString returns a simple string representation of a type expression. -func typeNameString(expr ast.Expr) string { - switch t := expr.(type) { - case *ast.Ident: - return t.Name - case *ast.SelectorExpr: - return typeNameString(t.X) + "." + t.Sel.Name - default: - return "" +// isExactContextType returns true if the type expression is exactly +// .SpecContext or .Context. +func isExactContextType(expr ast.Expr, ginkgoAlias, contextAlias string) bool { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return false } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + + if ginkgoAlias != "" && ident.Name == ginkgoAlias && sel.Sel.Name == "SpecContext" { + return true + } + if contextAlias != "" && ident.Name == contextAlias && sel.Sel.Name == "Context" { + return true + } + return false } From 93b5ba6e8d243354dd4feff8e3ae1a9ffea60106 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 9 Sep 2026 07:49:32 +0000 Subject: [PATCH 3/3] [ote] Move discovery verification into OTE module --- Makefile | 2 +- {hack => ote}/verify-ote-discovery.sh | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) rename {hack => ote}/verify-ote-discovery.sh (94%) diff --git a/Makefile b/Makefile index bbaa8d6ba0..85f6511fc6 100644 --- a/Makefile +++ b/Makefile @@ -54,7 +54,7 @@ verify: all verify-ote-discovery .PHONY: verify-ote-discovery verify-ote-discovery: - hack/verify-ote-discovery.sh + ote/verify-ote-discovery.sh ##@ General diff --git a/hack/verify-ote-discovery.sh b/ote/verify-ote-discovery.sh similarity index 94% rename from hack/verify-ote-discovery.sh rename to ote/verify-ote-discovery.sh index 18da4d5c27..844136e126 100755 --- a/hack/verify-ote-discovery.sh +++ b/ote/verify-ote-discovery.sh @@ -9,6 +9,12 @@ set -o pipefail WMCO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) cd "${WMCO_ROOT}" +# The k8s test framework requires KUBECONFIG to be set; create a stub +# so the AST test and binary can initialize without a real cluster connection. +FAKE_KUBECONFIG=$(mktemp) +trap 'rm -f -- "$FAKE_KUBECONFIG"' EXIT +export KUBECONFIG="${FAKE_KUBECONFIG}" + echo "==> Running OTE callback-signature static checks..." (cd ote && GOFLAGS="" GOWORK=off go test -v -run TestSpecTimeoutCallbackSignatures -count=1 ./cmd/wmco-tests-ext/) @@ -21,12 +27,6 @@ if [ ! -x "${BINARY}" ]; then exit 1 fi -# The k8s test framework requires KUBECONFIG to be set; create a stub -# so the binary can initialize without a real cluster connection. -FAKE_KUBECONFIG=$(mktemp) -trap 'rm -f -- "$FAKE_KUBECONFIG"' EXIT -export KUBECONFIG="${FAKE_KUBECONFIG}" - echo "==> Verifying OTE component registration (list components)..." COMPONENTS=$("${BINARY}" list components 2>&1) || { echo "ERROR: wmco-tests-ext list components failed"