From ffe70f1a6eeb5cc663a1edba8e03a91ece2c919a Mon Sep 17 00:00:00 2001
From: Aaron Crickenberger <spiffxp@google.com>
Date: Mon, 24 Jun 2019 09:35:12 -0700
Subject: [PATCH] Conformance walker should handle nested/adjacent Describes

This also revealed that the regex for Context was too generous and would
catch things like SecurityContext or ContextWithFoo(...) calls

This ensures that test suites with a ineligible tag in their top-level
Describe will be rejected from promotion to conformance
---
 test/conformance/walk.go      | 48 +++++++++++++++++++++++++----------
 test/conformance/walk_test.go | 19 +++++++++++++-
 2 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/test/conformance/walk.go b/test/conformance/walk.go
index a32e6df9a7b..366ca52fb0b 100644
--- a/test/conformance/walk.go
+++ b/test/conformance/walk.go
@@ -47,18 +47,19 @@ var (
 )
 
 const regexDescribe = "Describe|KubeDescribe|SIGDescribe"
-const regexContext = "Context"
+const regexContext = "^Context$"
 
 type visitor struct {
-	FileSet      *token.FileSet
-	lastDescribe describe
-	cMap         ast.CommentMap
+	FileSet   *token.FileSet
+	describes []describe
+	cMap      ast.CommentMap
 	//list of all the conformance tests in the path
 	tests []conformanceData
 }
 
 //describe contains text associated with ginkgo describe container
 type describe struct {
+	rparen      token.Pos
 	text        string
 	lastContext context
 }
@@ -202,7 +203,8 @@ func (v *visitor) emit(arg ast.Expr) {
 			return
 		}
 
-		err := validateTestName(v.getDescription(at.Value))
+		description := v.getDescription(at.Value)
+		err := validateTestName(description)
 		if err != nil {
 			v.failf(at, err.Error())
 			return
@@ -221,13 +223,21 @@ func (v *visitor) emit(arg ast.Expr) {
 }
 
 func (v *visitor) getDescription(value string) string {
-	if len(v.lastDescribe.lastContext.text) > 0 {
-		return strings.Trim(v.lastDescribe.text, "\"") +
-			" " + strings.Trim(v.lastDescribe.lastContext.text, "\"") +
-			" " + strings.Trim(value, "\"")
+	tokens := []string{}
+	for _, describe := range v.describes {
+		tokens = append(tokens, describe.text)
+		if len(describe.lastContext.text) > 0 {
+			tokens = append(tokens, describe.lastContext.text)
+		}
 	}
-	return strings.Trim(v.lastDescribe.text, "\"") +
-		" " + strings.Trim(value, "\"")
+	tokens = append(tokens, value)
+
+	trimmed := []string{}
+	for _, token := range tokens {
+		trimmed = append(trimmed, strings.Trim(token, "\""))
+	}
+
+	return strings.Join(trimmed, " ")
 }
 
 var (
@@ -321,12 +331,16 @@ func (v *visitor) matchFuncName(n *ast.CallExpr, pattern string) string {
 // It() with a manually embedded [Conformance] tag, which it will complain
 // about.
 func (v *visitor) Visit(node ast.Node) (w ast.Visitor) {
+	lastDescribe := len(v.describes) - 1
+
 	switch t := node.(type) {
 	case *ast.CallExpr:
 		if name := v.matchFuncName(t, regexDescribe); name != "" && len(t.Args) >= 2 {
-			v.lastDescribe = describe{text: name}
+			v.describes = append(v.describes, describe{text: name, rparen: t.Rparen})
 		} else if name := v.matchFuncName(t, regexContext); name != "" && len(t.Args) >= 2 {
-			v.lastDescribe.lastContext = context{text: name}
+			if lastDescribe > -1 {
+				v.describes[lastDescribe].lastContext = context{text: name}
+			}
 		} else if v.isConformanceCall(t) {
 			totalConfTests++
 			v.emit(t.Args[0])
@@ -337,6 +351,14 @@ func (v *visitor) Visit(node ast.Node) (w ast.Visitor) {
 			return nil
 		}
 	}
+
+	// If we're past the position of the last describe's rparen, pop the describe off
+	if lastDescribe > -1 && node != nil {
+		if node.Pos() > v.describes[lastDescribe].rparen {
+			v.describes = v.describes[:lastDescribe]
+		}
+	}
+
 	return v
 }
 
diff --git a/test/conformance/walk_test.go b/test/conformance/walk_test.go
index 2d3795d97ba..c18c3301adb 100644
--- a/test/conformance/walk_test.go
+++ b/test/conformance/walk_test.go
@@ -69,6 +69,23 @@ var _ = framework.KubeDescribe("Feature", func() {
 		Description: `By default the stdout and stderr from the process
 being executed in a pod MUST be sent to the pod's logs.` + "\n\n"}},
 	},
+	// SIGDescribe + KubeDescribe + It, Describe + KubeDescribe + It
+	{"e2e/foo.go", `
+var _ = framework.SIGDescribe("Feature", func() {
+	KubeDescribe("Described by", func() {
+		// Description: description1
+		framework.ConformanceIt("A ConformanceIt", func() {})
+	})
+	Describe("Also described via", func() {
+		KubeDescribe("A nested", func() {
+			// Description: description2
+			framework.ConformanceIt("ConformanceIt", func() {})
+		})
+	})
+})`, []conformanceData{
+		{URL: "https://github.com/kubernetes/kubernetes/tree/master/e2e/foo.go#L6", TestName: "Feature Described by A ConformanceIt", Description: "description1\n\n"},
+		{URL: "https://github.com/kubernetes/kubernetes/tree/master/e2e/foo.go#L11", TestName: "Feature Also described via A nested ConformanceIt", Description: "description2\n\n"},
+	}},
 	// KubeDescribe + Context + It
 	{"e2e/foo.go", `
 var _ = framework.KubeDescribe("Feature", func() {
@@ -101,7 +118,7 @@ func TestConformance(t *testing.T) {
 		*confDoc = true
 		tests := scanfile(test.filename, code)
 		if !reflect.DeepEqual(tests, test.output) {
-			t.Errorf("code:\n%s\ngot  %v\nwant %v",
+			t.Errorf("code:\n%s\ngot  %+v\nwant %+v",
 				code, tests, test.output)
 		}
 	}