Pass go test flags to integration tests (#3109)

* Pass go test flags to integration tests
* Update docs/test-framework-dev-guide.md
* Add clarification about ExtraFlags and RunExpr ordering in devtools.GoTest

---------

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

dev-tools/mage/gotest.go

@@ -232,11 +232,26 @@ func GoTest(ctx context.Context, params GoTestArgs) error {
 		)
 	}
 
+	// Pass the go test extra flags BEFORE the RunExpr.
+	// TL;DR: This is needed to make sure that a -test.run flag specified in the GOTEST_FLAGS environment variable does
+	// not interfere with the batching done by the framework.
+	//
+	// Full explanation:
+	// The integration test framework runs the tests twice:
+	// - the first time we pass a special tag that make all the define statements in the tests skip the test and dump the requirements.
+	//   This output is processed by the integration test framework to discover the tests and the set of environments/machines
+	//   we will need to spawn and allocate the tests to the various machines. (see batch.go for details)
+	// - the second time we run the tests (here) the integration test framework adds a -test.run flag when launching go test
+	//   on the remote machine to make sure that only the tests corresponding to that batch are executed.
+	//
+	// By specifying the extra flags before the -test.run for the batch we make sure that the last flag definition "wins"
+	// (have a look at the unit test in batch_test.go), so that whatever run constraint is specified in GOTEST_FLAGS
+	// participates in the discovery and batching (1st go test execution) but doesn't override the actual execution on
+	// the remote machine (2nd go test execution).
+	testArgs = append(testArgs, params.ExtraFlags...)
 	if params.RunExpr != "" {
 		testArgs = append(testArgs, "-run", params.RunExpr)
 	}
-
-	testArgs = append(testArgs, params.ExtraFlags...)
 	testArgs = append(testArgs, params.Packages...)
 
 	args := append(gotestsumArgs, append([]string{"--"}, testArgs...)...)

docs/test-framework-dev-guide.md

@@ -49,6 +49,42 @@ pass `[testName]` to `go test` as `--run=[testName]`.
 
 - `mage integration:matrix` to run all tests on the complete matrix of supported operating systems and architectures of the Elastic Agent.
 
+#### Passing additional go test flags
+
+When running the tests we can pass additional go test flag using the env variable `GOTEST_FLAGS`.
+
+These flags are passed also when calculating batches for remote execution of integration tests.
+This allows for selecting a subset of test in a convenient way (see examples below)
+
+This feature is intended mostly for integration tests debugging/development without the need for
+new mage targets corresponding to a new set of test flags.
+
+A few examples:
+
+##### Run a single test with an exact match
+We want to run only the test named "TestStandaloneUpgrade"
+`GOTEST_FLAGS="-test.run ^TestStandaloneUpgrade$" mage integration:test`
+
+##### Run a tests matching a partial expression
+We want to run any test with "Upgrade" in the name
+`GOTEST_FLAGS="-test.run Upgrade" mage integration:test`
+
+##### Run a single test and signal that we want the short version
+We pass a `-test.short` flag along with the name match
+`GOTEST_FLAGS="-test.run ^TestStandaloneUpgrade$ -test.short" mage integration:test`
+
+##### Run a single test multiple times
+We pass a `-test.count` flag along with the name match
+`GOTEST_FLAGS="-test.run ^TestStandaloneUpgrade$ -test.count 10" mage integration:test`
+
+##### Run specific tests
+We pass a `-test.run` flag along with the names of the tests we want to run in OR
+`GOTEST_FLAGS="-test.run ^(TestStandaloneUpgrade|TestFleetManagedUpgrade)$" mage integration:test`
+
+##### Limitations
+Due to the way the parameters are passed to `devtools.GoTest` the value of the environment variable
+is split on space, so not all combination of flags and their values may be correctly split.
+
 ## Writing tests
 
 Write integration and E2E tests by adding them to the `testing/integration`

magefile.go

@@ -1299,7 +1299,7 @@ func (Integration) Clean() error {
 	_, err := os.Stat(".ogc-cache")
 	if err == nil {
 		// .ogc-cache exists; need to run `Clean` from the runner
-		r, err := createTestRunner(false, "")
+		r, err := createTestRunner(false, "", "")
 		if err != nil {
 			return fmt.Errorf("error creating test runner: %w", err)
 		}
@@ -1335,6 +1335,10 @@ func (Integration) Local(ctx context.Context, testName string) error {
 	params := devtools.DefaultGoTestIntegrationArgs()
 	params.Tags = append(params.Tags, "local")
 	params.Packages = []string{"github.com/elastic/elastic-agent/testing/integration"}
+
+	goTestFlags := strings.SplitN(os.Getenv("GOTEST_FLAGS"), " ", -1)
+	params.ExtraFlags = goTestFlags
+
 	if testName == "all" {
 		params.RunExpr = ""
 	} else {
@@ -1400,6 +1404,13 @@ func (Integration) TestOnRemote(ctx context.Context) error {
 	if testsStr == "" {
 		return errors.New("TEST_DEFINE_TESTS environment variable must be set")
 	}
+
+	var goTestFlags []string
+	rawTestFlags := os.Getenv("GOTEST_FLAGS")
+	if rawTestFlags != "" {
+		goTestFlags = strings.Split(rawTestFlags, " ")
+	}
+
 	tests := strings.Split(testsStr, ",")
 	testsByPackage := make(map[string][]string)
 	for _, testStr := range tests {
@@ -1425,17 +1436,19 @@ func (Integration) TestOnRemote(ctx context.Context) error {
 		testPrefix := fmt.Sprintf("%s.%s", prefix, filepath.Base(packageName))
 		testName := fmt.Sprintf("remote-%s", testPrefix)
 		fileName := fmt.Sprintf("build/TEST-go-%s", testName)
+		extraFlags := make([]string, 0, len(goTestFlags)+6)
+		if len(goTestFlags) > 0 {
+			extraFlags = append(extraFlags, goTestFlags...)
+		}
+		extraFlags = append(extraFlags, "-test.shuffle", "on",
+			"-test.timeout", "0", "-test.run", "^("+strings.Join(packageTests, "|")+")$")
 		params := mage.GoTestArgs{
 			LogName:         testName,
 			OutputFile:      fileName + ".out",
 			JUnitReportFile: fileName + ".xml",
 			Packages:        []string{packageName},
 			Tags:            []string{"integration"},
-			ExtraFlags: []string{
-				"-test.run", strings.Join(packageTests, "|"),
-				"-test.shuffle", "on",
-				"-test.timeout", "0",
-			},
+			ExtraFlags:      extraFlags,
 			Env: map[string]string{
 				"AGENT_VERSION":      version,
 				"TEST_DEFINE_PREFIX": testPrefix,
@@ -1450,11 +1463,13 @@ func (Integration) TestOnRemote(ctx context.Context) error {
 }
 
 func integRunner(ctx context.Context, matrix bool, singleTest string) error {
-	batches, err := define.DetermineBatches("testing/integration", "integration")
+	goTestFlags := os.Getenv("GOTEST_FLAGS")
+
+	batches, err := define.DetermineBatches("testing/integration", goTestFlags, "integration")
 	if err != nil {
 		return fmt.Errorf("failed to determine batches: %w", err)
 	}
-	r, err := createTestRunner(matrix, singleTest, batches...)
+	r, err := createTestRunner(matrix, singleTest, goTestFlags, batches...)
 	if err != nil {
 		return fmt.Errorf("error creating test runner: %w", err)
 	}
@@ -1491,7 +1506,7 @@ func integRunner(ctx context.Context, matrix bool, singleTest string) error {
 	return nil
 }
 
-func createTestRunner(matrix bool, singleTest string, batches ...define.Batch) (*runner.Runner, error) {
+func createTestRunner(matrix bool, singleTest string, goTestFlags string, batches ...define.Batch) (*runner.Runner, error) {
 	goVersion, err := mage.DefaultBeatBuildVariableSources.GetGoVersion()
 	if err != nil {
 		return nil, err
@@ -1561,6 +1576,7 @@ func createTestRunner(matrix bool, singleTest string, batches ...define.Batch) (
 		SingleTest:  singleTest,
 		VerboseMode: mg.Verbose(),
 		Timestamp:   timestamp,
+		TestFlags:   goTestFlags,
 	}, batches...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create runner: %w", err)

pkg/testing/define/batch.go

@@ -69,7 +69,7 @@ type BatchPackageTests struct {
 
 // DetermineBatches parses the package directory with the possible extra build
 // tags to determine the set of batches for the package.
-func DetermineBatches(dir string, buildTags ...string) ([]Batch, error) {
+func DetermineBatches(dir string, testFlags string, buildTags ...string) ([]Batch, error) {
 	const (
 		defineMatcher = "define skip; requirements: "
 	)
@@ -86,7 +86,13 @@ func DetermineBatches(dir string, buildTags ...string) ([]Batch, error) {
 
 	// run 'go test' and collect the JSON output to be parsed
 	// #nosec G204 -- test function code, it will be okay
-	cmdArgs := []string{"test", "-v", "--tags", strings.Join(buildTags, ","), "-json", dir}
+	cmdArgs := []string{"test", "-v", "--tags", strings.Join(buildTags, ","), "-json"}
+	if testFlags != "" {
+		flags := strings.Split(testFlags, " ")
+		cmdArgs = append(cmdArgs, flags...)
+	}
+
+	cmdArgs = append(cmdArgs, dir)
 	testCmd := exec.Command("go", cmdArgs...)
 	output, err := testCmd.Output()
 	if err != nil {
@@ -202,6 +208,13 @@ func appendTest(batches []Batch, tar testActionResult, req Requirements) []Batch
 func appendPackageTest(tests []BatchPackageTests, pkg string, name string) []BatchPackageTests {
 	for i, pt := range tests {
 		if pt.Name == pkg {
+			for _, testName := range pt.Tests {
+				if testName == name {
+					// we already selected this test for this package for this batch,
+					// we can return immediately
+					return tests
+				}
+			}
 			pt.Tests = append(pt.Tests, name)
 			tests[i] = pt
 			return tests

pkg/testing/define/batch_test.go

@@ -276,7 +276,84 @@ func TestBatch(t *testing.T) {
 		},
 	}
 
-	actual, err := DetermineBatches("testdata", "batch_test")
+	actual, err := DetermineBatches("testdata", "", "batch_test")
 	require.NoError(t, err)
 	require.EqualValues(t, expected, actual)
 }
+
+var testLinuxLocalTests = []string{"TestLinuxLocal"}
+
+var testLinuxLocalBatch = []Batch{
+	{
+		OS: OS{
+			Type: "linux",
+			Arch: "amd64",
+		},
+		Tests: []BatchPackageTests{
+			{
+				Name:  "github.com/elastic/elastic-agent/pkg/testing/define/testdata",
+				Tests: testLinuxLocalTests,
+			},
+		},
+	},
+	{
+		OS: OS{
+			Type: "linux",
+			Arch: "arm64",
+		},
+		Tests: []BatchPackageTests{
+			{
+				Name:  "github.com/elastic/elastic-agent/pkg/testing/define/testdata",
+				Tests: testLinuxLocalTests,
+			},
+		},
+	},
+}
+
+func TestGoTestFlags(t *testing.T) {
+	testcases := []struct {
+		name     string
+		flags    string
+		expected []Batch
+	}{
+		{
+			name:     "Run single test",
+			flags:    "-run ^TestLinuxLocal$",
+			expected: testLinuxLocalBatch,
+		},
+		{
+			name:     "Run single test with short flag",
+			flags:    "-run ^TestLinuxLocal$ -short",
+			expected: testLinuxLocalBatch,
+		},
+		{
+			name:     "specify non-existing test",
+			flags:    "-run ^thisdoesnotexist$",
+			expected: nil,
+		},
+		{
+			name:     "specify multiple run flags - last one wins - no test",
+			flags:    "-run ^TestLinuxLocal$ -run ^thisdoesnotexist$",
+			expected: nil,
+		},
+		{
+			name:     "specify multiple run flags - last one wins - TestLinuxLocal",
+			flags:    "-run ^thisdoesnotexist$ -run ^TestLinuxLocal$",
+			expected: testLinuxLocalBatch,
+		},
+		{
+			name:     "count flag will not multiply the test entries in each batch",
+			flags:    "-run ^TestLinuxLocal$ -count 2",
+			expected: testLinuxLocalBatch,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual, err := DetermineBatches("testdata", tc.flags, "batch_test")
+			require.NoError(t, err)
+			require.EqualValues(t, tc.expected, actual)
+		})
+	}
+
+}

pkg/testing/runner/config.go

@@ -33,6 +33,9 @@ type Config struct {
 
 	// Timestamp enables timestamps on the console output.
 	Timestamp bool
+
+	// Testflags contains extra go test flags to be set when running tests
+	TestFlags string
 }
 
 // Validate returns an error if the information is invalid.

pkg/testing/runner/runner.go

@@ -264,7 +264,7 @@ func (r *Runner) runMachine(ctx context.Context, sshAuth ssh.AuthMethod, logger
 	}
 
 	// ensure that we have all the requirements for the stack if required
-	var env map[string]string
+	env := map[string]string{}
 	if batch.Batch.Stack != nil {
 		ch, err := r.getCloudForBatchID(batch.ID)
 		if err != nil {
@@ -276,18 +276,19 @@ func (r *Runner) runMachine(ctx context.Context, sshAuth ssh.AuthMethod, logger
 			return OSRunnerResult{}, fmt.Errorf("cannot continue because stack never became ready")
 		}
 		logger.Logf("Will continue, stack is ready")
-		env = map[string]string{
-			"ELASTICSEARCH_HOST":     resp.ElasticsearchEndpoint,
-			"ELASTICSEARCH_USERNAME": resp.Username,
-			"ELASTICSEARCH_PASSWORD": resp.Password,
-			"KIBANA_HOST":            resp.KibanaEndpoint,
-			"KIBANA_USERNAME":        resp.Username,
-			"KIBANA_PASSWORD":        resp.Password,
-		}
-		logger.Logf("Created Stack with Kibana host %s, %s/%s", resp.KibanaEndpoint, resp.Username, resp.Password)
+		env["ELASTICSEARCH_HOST"] = resp.ElasticsearchEndpoint
+		env["ELASTICSEARCH_USERNAME"] = resp.Username
+		env["ELASTICSEARCH_PASSWORD"] = resp.Password
+		env["KIBANA_HOST"] = resp.KibanaEndpoint
+		env["KIBANA_USERNAME"] = resp.Username
+		env["KIBANA_PASSWORD"] = resp.Password
 
+		logger.Logf("Created Stack with Kibana host %s, %s/%s", resp.KibanaEndpoint, resp.Username, resp.Password)
 	}
 
+	// set the go test flags
+	env["GOTEST_FLAGS"] = r.cfg.TestFlags
+
 	// run the actual tests on the host
 	prefix := fmt.Sprintf("%s-%s-%s-%s", batch.LayoutOS.OS.Type, batch.LayoutOS.OS.Arch, batch.LayoutOS.OS.Distro, strings.Replace(batch.LayoutOS.OS.Version, ".", "", -1))
 	result, err := batch.LayoutOS.Runner.Run(ctx, r.cfg.VerboseMode, client, logger, r.cfg.AgentVersion, prefix, batch.Batch, env)