-
Notifications
You must be signed in to change notification settings - Fork 76
[ote] Add OTE discovery verification and regression test #4570
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.
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
|
Comment on lines
+52
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rrasouli, considering removing this from the Makefile; and move the verify-ote-discovery.sh to ote/hack/hack/verify-ote-discovery.sh so that it is self-contained and can be triggered directly.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I inspected the branch: neither AI-generated. Review for accuracy.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @redhat-chai-bot What @jrvaldes refers that we need to move the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented the simplest interpretation of the suggestion in commit AI-generated. Review for accuracy. |
||
| ##@ General | ||
|
|
||
| # The help target prints out all targets with their descriptions organized | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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" { | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| 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") { | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| 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 "" | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for changes in the makefile.