feat(fluxtest): enhance fluxtest to use package name with test/skip f…

…lags (#5039)

* feat(fluxtest): enhance fluxtest to use package name with test and skip flags

Conflicting test names produce an error
Existing conflict are fixed

fixes: #4877

* fix: fixed package name in error

* fix: add dot between package and testname

* feat: add location in the duplicate test message

* test: added test for duplicate testcase name

* fix: allow only dot as a package qualifier

* chore: rebased to master

* fix: gen getting auto formatted with extra line

ast/ast.go

@@ -1117,7 +1117,8 @@ func (e *FunctionExpression) Copy() Node {
 // Result of evaluating an equality operator is always of type Boolean based on whether the
 // comparison is true
 // Arithmetic operators take numerical values (either literals or variables) as their operands
-//  and return a single numerical value.
+//
+//	and return a single numerical value.
 type OperatorKind int
 
 const (
@@ -1263,7 +1264,7 @@ func (o *LogicalOperatorKind) UnmarshalText(data []byte) error {
 
 // LogicalExpression represent the rule conditions that collectively evaluate to either true or false.
 // `or` expressions compute the disjunction of two boolean expressions and return boolean values.
-// `and`` expressions compute the conjunction of two boolean expressions and return boolean values.
+// `and“ expressions compute the conjunction of two boolean expressions and return boolean values.
 type LogicalExpression struct {
 	BaseNode
 	Operator LogicalOperatorKind `json:"operator"`

ast/edit/option_editor.go

@@ -76,9 +76,9 @@ func OptionObjectFn(keyMap map[string]ast.Expression) OptionFn {
 	}
 }
 
-//Finds the `OptionStatement` with the specified `identifier` and updates its value.
-//There shouldn't be more then one option statement with the same identifier
-//in a valid query.
+// Finds the `OptionStatement` with the specified `identifier` and updates its value.
+// There shouldn't be more then one option statement with the same identifier
+// in a valid query.
 type optionEditor struct {
 	identifier string
 	optionFn   OptionFn

ast/testcase/testcase.go

@@ -18,17 +18,17 @@ import (
 //
 // A testcase is defined with the testcase statement such as below.
 //
-//     import "testing/assert"
-//     myVar = 4
-//     testcase addition {
-//         assert.equal(want: 2 + 2, got: myVar)
-//     }
+//	import "testing/assert"
+//	myVar = 4
+//	testcase addition {
+//	    assert.equal(want: 2 + 2, got: myVar)
+//	}
 //
 // This gets transformed into a package that looks like this:
 //
-//     import "testing/assert"
-//     myVar = 4
-//     assert.equal(want: 2 + 2, got: myVar)
+//	import "testing/assert"
+//	myVar = 4
+//	assert.equal(want: 2 + 2, got: myVar)
 //
 // It is allowed to include options within the testcase block as they will be extracted
 // to the top level.
@@ -37,23 +37,22 @@ import (
 // This will transform the the extended testcase in a slightly different way.
 // The syntax for extending is as such:
 //
-//     import "math"
-//     testcase addition_v2 extends "math_test.addition" {
-//         option math.enable_v2 = true
-//         super()
-//     }
+//	import "math"
+//	testcase addition_v2 extends "math_test.addition" {
+//	    option math.enable_v2 = true
+//	    super()
+//	}
 //
 // The extending test case is then transformed into a single file combining both the parent
 // statements and the current statements.
 //
-//     import "testing/assert"
-//     import "math"
+//	import "testing/assert"
+//	import "math"
 //
-//     option math.enable_v2 = true
-//
-//     myVar = 4
-//     assert.equal(want: 2 + 2, got: myVar)
+//	option math.enable_v2 = true
 //
+//	myVar = 4
+//	assert.equal(want: 2 + 2, got: myVar)
 //
 // The call to `super()` is replaced with the body of the parent test case.
 //
@@ -62,7 +61,7 @@ import (
 // It is allowed for an imported testcase to have an option, but no attempt is made
 // to remove duplicate options. If there is a duplicate option, this will likely
 // cause an error when the test is actually run.
-func Transform(ctx context.Context, pkg *ast.Package, modules TestModules) ([]string, []*ast.Package, error) {
+func Transform(ctx context.Context, pkg *ast.Package, modules TestModules) ([]*ast.Identifier, []*ast.Package, error) {
 	if len(pkg.Files) != 1 {
 		return nil, nil, errors.Newf(codes.FailedPrecondition, "unsupported number of files in test case package, got %d", len(pkg.Files))
 	}
@@ -81,7 +80,7 @@ func Transform(ctx context.Context, pkg *ast.Package, modules TestModules) ([]st
 	}
 
 	var (
-		names = make([]string, 0, n)
+		idens = make([]*ast.Identifier, 0, n)
 		pkgs  = make([]*ast.Package, 0, n)
 	)
 	for _, item := range file.Body {
@@ -94,11 +93,11 @@ func Transform(ctx context.Context, pkg *ast.Package, modules TestModules) ([]st
 		if err != nil {
 			return nil, nil, err
 		}
-		names = append(names, testcase.ID.Name)
+		idens = append(idens, testcase.ID)
 		pkgs = append(pkgs, testpkg)
 	}
 
-	return names, pkgs, nil
+	return idens, pkgs, nil
 }
 
 func newTestPackage(ctx context.Context, basePkg *ast.Package, preamble []ast.Statement, tc *ast.TestCaseStatement, modules TestModules) (*ast.Package, error) {

ast/testcase/testcase_test.go

@@ -51,12 +51,16 @@ testcase test_subtraction {
 
 	d := parser.ParseSource(testFile)
 
-	names, transformed, err := testcase.Transform(context.Background(), d, nil)
+	idens, transformed, err := testcase.Transform(context.Background(), d, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
+	testNames := make([]string, len(idens))
+	for i := range idens {
+		testNames[i] = idens[i].Name
+	}
 
-	if want, got := []string{"test_addition", "test_subtraction"}, names; !cmp.Equal(want, got) {
+	if want, got := []string{"test_addition", "test_subtraction"}, testNames; !cmp.Equal(want, got) {
 		t.Errorf("unexpected test names: -want/+got:\n%s", cmp.Diff(want, got))
 	}
 	if !cmp.Equal(expected, transformed, asttest.IgnoreBaseNodeOptions...) {
@@ -114,16 +118,19 @@ testcase b extends "flux/a/a_test.a" {
 	}
 	pkg.Files[0].Name = "b/b_test.flux"
 
-	names, pkgs, err := testcase.Transform(ctx, pkg, testcase.TestModules{
+	idens, pkgs, err := testcase.Transform(ctx, pkg, testcase.TestModules{
 		"flux": testcase.TestModule{
 			Service: fs,
 		},
 	})
 	if err != nil {
 		t.Fatalf("unexpected error: %s", err)
 	}
-
-	if want, got := []string{"b"}, names; !cmp.Equal(want, got) {
+	testNames := make([]string, len(idens))
+	for i := range idens {
+		testNames[i] = idens[i].Name
+	}
+	if want, got := []string{"b"}, testNames; !cmp.Equal(want, got) {
 		t.Fatalf("unexpected testcase names -want/+got:\n%s", cmp.Diff(want, got))
 	}
 

cmd/flux/cmd/test.go

@@ -165,17 +165,20 @@ type Test struct {
 	ast  *ast.Package
 	// set of tags specified for the test case
 	tags []string
+	// set package name for the test case
+	pkg string
 	// indicates if the test should be skipped
 	skip bool
 	err  error
 }
 
 // NewTest creates a new Test instance from an ast.Package.
-func NewTest(name string, ast *ast.Package, tags []string) Test {
+func NewTest(name string, ast *ast.Package, tags []string, pkg string) Test {
 	return Test{
 		name: name,
 		ast:  ast,
 		tags: tags,
+		pkg:  pkg,
 	}
 }
 
@@ -188,6 +191,10 @@ func (t *Test) Name() string {
 	return t.name
 }
 
+func (t *Test) PackageName() string {
+	return t.pkg
+}
+
 // Get the error from the test, if one exists.
 func (t *Test) Error() error {
 	return t.err
@@ -260,6 +267,33 @@ func contains(names []string, name string) bool {
 	return false
 }
 
+func containsWithPkgName(names []string, test *Test) bool {
+	var aTest, tTestName string
+	for _, name := range names {
+		fTest := splitAny(name, ".")
+		// handle package.TestName or package/TestName case
+		if len(fTest) > 1 {
+			aTest = strings.Join(fTest, ".")
+			tTestName = test.PackageName() + "." + test.Name()
+		} else {
+			aTest = name
+			tTestName = test.Name()
+		}
+		if aTest == tTestName {
+			return true
+		}
+	}
+	return false
+}
+
+// split a string by multiple runes
+func splitAny(s string, seps string) []string {
+	splitter := func(r rune) bool {
+		return strings.ContainsRune(seps, r)
+	}
+	return strings.FieldsFunc(s, splitter)
+}
+
 // TestRunner gathers and runs all tests.
 type TestRunner struct {
 	tests     []*Test
@@ -317,6 +351,11 @@ func (t *TestRunner) Gather(roots []string) error {
 		}
 
 		ctx := filesystem.Inject(context.Background(), fs)
+		// check for the duplicate testcase names
+		type testcaseLoc struct {
+			loc *ast.SourceLocation
+		}
+		seen := make(map[string]testcaseLoc)
 		for _, file := range files {
 			q, err := filesystem.ReadFile(ctx, file.path)
 			if err != nil {
@@ -326,20 +365,26 @@ func (t *TestRunner) Gather(roots []string) error {
 			if len(baseAST.Files) > 0 {
 				baseAST.Files[0].Name = file.path
 			}
-			tcnames, asts, err := testcase.Transform(ctx, baseAST, modules)
+			tcidens, asts, err := testcase.Transform(ctx, baseAST, modules)
 			if err != nil {
 				return err
 			}
+			pkg := strings.TrimSuffix(baseAST.Package, "_test")
 			for i, astf := range asts {
 				tags, err := readTags(astf)
 				if err != nil {
 					return err
 				}
 				if invalid := invalidTags(tags, mods.Tags(file.module)); len(invalid) != 0 {
-					return errors.Newf(codes.Invalid, "testcase %q, contains invalid tags %v, valid tags are: %v", tcnames[i], invalid, mods.Tags(file.module))
+					return errors.Newf(codes.Invalid, "testcase %q, contains invalid tags %v, valid tags are: %v", tcidens[i].Name, invalid, mods.Tags(file.module))
+				}
+				pkgTest := pkg + "." + tcidens[i].Name
+				if _, ok := seen[pkgTest]; ok {
+					return errors.Newf(codes.AlreadyExists, "duplicate testcase name %q, found in package %q, at locations %v and %v", tcidens[i].Name, pkg, seen[pkgTest].loc.String(), tcidens[i].Loc.String())
 				}
-				test := NewTest(tcnames[i], astf, tags)
+				test := NewTest(tcidens[i].Name, astf, tags, pkg)
 				t.tests = append(t.tests, &test)
+				seen[pkgTest] = testcaseLoc{tcidens[i].Loc}
 			}
 		}
 	}
@@ -370,26 +415,21 @@ func union(a, b []string) []string {
 // MarkSkipped checks the provided filters and marks each test case as skipped as needed.
 //
 // Skip rules:
-//  - When testNames is not empty any test in the list will be run, all others skipped.
-//  - When a test name is in skips, the test is skipped.
-//  - When a test contains any tags all tags must be specified for the test to run.
-//  - When skipUntagged is true, any test that does not have any tags is skipped.
+//   - When testNames is not empty any test in the list will be run, all others skipped.
+//   - When a test name is in skips, the test is skipped.
+//   - When a test contains any tags all tags must be specified for the test to run.
+//   - When skipUntagged is true, any test that does not have any tags is skipped.
 //
 // The list of tests takes precedence over all other parameters.
 func (t *TestRunner) MarkSkipped(testNames, skips, tags []string, skipUntagged bool) {
-	skipMap := make(map[string]bool)
-	for _, n := range skips {
-		skipMap[n] = true
-	}
-
 	for i := range t.tests {
 		// If testNames is not empty then check only that list
 		if len(testNames) > 0 {
-			t.tests[i].skip = !contains(testNames, t.tests[i].Name())
+			t.tests[i].skip = !containsWithPkgName(testNames, t.tests[i])
 			continue
 		}
 		// Now we assume the test is not skipped and check the rest of the rules
-		skip := false
+		skipBecauseTags := false
 
 		if len(t.tests[i].tags) > 0 {
 			// Tags must be present for all test tags
@@ -398,10 +438,16 @@ func (t *TestRunner) MarkSkipped(testNames, skips, tags []string, skipUntagged b
 				isMatch = isMatch && contains(tags, tag)
 			}
 			if !isMatch {
-				skip = true
+				skipBecauseTags = true
 			}
 		}
-		t.tests[i].skip = skip || skipMap[t.tests[i].Name()] || (skipUntagged && len(t.tests[i].tags) == 0)
+		// skip tests coming from skip list
+		skipBecauseSkipList := false
+		if !skipBecauseTags && len(skips) > 0 {
+			skipBecauseSkipList = containsWithPkgName(skips, t.tests[i])
+		}
+
+		t.tests[i].skip = skipBecauseTags || skipBecauseSkipList || (skipUntagged && len(t.tests[i].tags) == 0)
 	}
 }