From 338fe7ad55cad858284d4c96df86d3cee9c7d29c Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Wed, 5 Jul 2023 15:44:11 +0200
Subject: [PATCH] e2e framework: validate test definitions

This checks that the With* label functions are used instead of the previous
inline tags. To catch strings passed to Ginkgo directly instead of the
framework wrapper functions, the final test specs are checked.
---
 test/e2e/framework/ginkgowrapper.go | 78 +++++++++++++++++++++++++++++
 test/e2e/framework/test_context.go  |  1 +
 2 files changed, 79 insertions(+)

diff --git a/test/e2e/framework/ginkgowrapper.go b/test/e2e/framework/ginkgowrapper.go
index 01d226a578b..e04eeff74dc 100644
--- a/test/e2e/framework/ginkgowrapper.go
+++ b/test/e2e/framework/ginkgowrapper.go
@@ -246,6 +246,84 @@ func registerInSuite(ginkgoCall func(string, ...interface{}) bool, args []interf
 	return ginkgoCall(text, ginkgoArgs...)
 }
 
+var (
+	tagRe                 = regexp.MustCompile(`\[.*?\]`)
+	deprecatedTags        = sets.New("Conformance", "NodeConformance", "Disruptive", "Serial", "Slow")
+	deprecatedTagPrefixes = sets.New("Environment", "Feature", "NodeFeature", "FeatureGate")
+	deprecatedStability   = sets.New("Alpha", "Beta")
+)
+
+// validateSpecs checks that the test specs were registered as intended.
+func validateSpecs(specs types.SpecReports) {
+	checked := sets.New[call]()
+
+	for _, spec := range specs {
+		for i, text := range spec.ContainerHierarchyTexts {
+			c := call{
+				text:     text,
+				location: spec.ContainerHierarchyLocations[i],
+			}
+			if checked.Has(c) {
+				// No need to check the same container more than once.
+				continue
+			}
+			checked.Insert(c)
+			validateText(c.location, text, spec.ContainerHierarchyLabels[i])
+		}
+		c := call{
+			text:     spec.LeafNodeText,
+			location: spec.LeafNodeLocation,
+		}
+		if !checked.Has(c) {
+			validateText(spec.LeafNodeLocation, spec.LeafNodeText, spec.LeafNodeLabels)
+			checked.Insert(c)
+		}
+	}
+}
+
+// call acts as (mostly) unique identifier for a container node call like
+// Describe or Context. It's not perfect because theoretically a line might
+// have multiple calls with the same text, but that isn't a problem in
+// practice.
+type call struct {
+	text     string
+	location types.CodeLocation
+}
+
+// validateText checks for some known tags that should not be added through the
+// plain text strings anymore. Eventually, all such tags should get replaced
+// with the new APIs.
+func validateText(location types.CodeLocation, text string, labels []string) {
+	for _, tag := range tagRe.FindAllString(text, -1) {
+		if tag == "[]" {
+			recordTextBug(location, "[] in plain text is invalid")
+			continue
+		}
+		// Strip square brackets.
+		tag = tag[1 : len(tag)-1]
+		if slices.Contains(labels, tag) {
+			// Okay, was also set as label.
+			continue
+		}
+		if deprecatedTags.Has(tag) {
+			recordTextBug(location, fmt.Sprintf("[%s] in plain text is deprecated and must be added through With%s instead", tag, tag))
+		}
+		if deprecatedStability.Has(tag) {
+			recordTextBug(location, fmt.Sprintf("[%s] in plain text is deprecated and must be added by defining the feature gate through WithFeatureGate instead", tag))
+		}
+		if index := strings.Index(tag, ":"); index > 0 {
+			prefix := tag[:index]
+			if deprecatedTagPrefixes.Has(prefix) {
+				recordTextBug(location, fmt.Sprintf("[%s] in plain text is deprecated and must be added through With%s(%s) instead", tag, prefix, tag[index+1:]))
+			}
+		}
+	}
+}
+
+func recordTextBug(location types.CodeLocation, message string) {
+	RecordBug(Bug{FileName: location.FileName, LineNumber: location.LineNumber, Message: message})
+}
+
 // WithEnvironment specifies that a certain test or group of tests only works
 // with a feature available. The return value must be passed as additional
 // argument to [framework.It], [framework.Describe], [framework.Context].
diff --git a/test/e2e/framework/test_context.go b/test/e2e/framework/test_context.go
index a95f50c7dc1..106b62d3104 100644
--- a/test/e2e/framework/test_context.go
+++ b/test/e2e/framework/test_context.go
@@ -517,6 +517,7 @@ func AfterReadingAllFlags(t *TestContextType) {
 
 	// ginkgo.PreviewSpecs will expand all nodes and thus may find new bugs.
 	report := ginkgo.PreviewSpecs("Kubernetes e2e test statistics")
+	validateSpecs(report.SpecReports)
 	if err := FormatBugs(); CheckForBugs && err != nil {
 		// Refuse to do anything if the E2E suite is buggy.
 		fmt.Fprint(Output, "ERROR: E2E suite initialization was faulty, these errors must be fixed:")