Ignore AGENT_KEEP_INSTALLED if test succeeds (elastic#3348) (elastic#…

…3379)

If the agent is kept installed after a successful test, other tests installing the agent might fail.

(cherry picked from commit 469cb1c)

Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>

docs/test-framework-dev-guide.md

@@ -128,9 +128,12 @@ every tests you can use the environment variable `AGENT_COLLECT_DIAG=true`. When
 it will cause the testing fixture to always collect diagnostics before the uninstall in the cleanup step of a test.
 
 #### Keeping Elastic Agent installed
-When the testing fixture installs the Elastic Agent it will automatically uninstall the Elastic Agent during the
-cleanup process of the test. In the case that you do not want that to happen you can disable the auto-uninstallation
-using `AGENT_KEEP_INSTALLED=true` environment variable. It is recommend to only do this when inspecting a single test.
+
+When the testing fixture installs the Elastic Agent, it will automatically uninstall
+the Elastic Agent during the cleanup process of the test. In the event that you do
+not want this to occur, you can disable the auto-uninstallation using the
+AGENT_KEEP_INSTALLED=true environment variable. If the test succeeds, the agent
+will be uninstalled regardless of the value of AGENT_KEEP_INSTALLED.
 
 - `AGENT_KEEP_INSTALLED=true mage integration:single [testName]`
 

pkg/testing/fixture_install.go

@@ -109,13 +109,14 @@ func (f *Fixture) Install(ctx context.Context, installOpts *InstallOpts, opts ..
 
 	f.t.Cleanup(func() {
 		if !f.installed {
+			f.t.Logf("skipping uninstall; agent not installed (fixture.installed is false)")
 			// not installed; no need to clean up or collect diagnostics
 			return
 		}
 
 		// diagnostics is collected when either the environment variable
-		// AGENT_KEEP_INSTALLED=true or the test is marked failed
-		collect := collectDiag()
+		// AGENT_COLLECT_DIAG=true or the test is marked failed
+		collect := collectDiagFlag()
 		failed := f.t.Failed()
 		if collect || failed {
 			if collect {
@@ -128,23 +129,28 @@ func (f *Fixture) Install(ctx context.Context, installOpts *InstallOpts, opts ..
 
 		// environment variable AGENT_KEEP_INSTALLED=true will skip the uninstall
 		// useful to debug the issue with the Elastic Agent
-		if keepInstalled() {
+		if keepInstalled() && f.t.Failed() {
 			f.t.Logf("skipping uninstall; AGENT_KEEP_INSTALLED=true")
+			return
 		} else {
-			out, err := f.Uninstall(ctx, &UninstallOpts{Force: true, UninstallToken: f.uninstallToken})
-			f.setClient(nil)
-			if err != nil &&
-				(errors.Is(err, ErrNotInstalled) ||
-					strings.Contains(
-						err.Error(),
-						"elastic-agent: no such file or directory")) {
-				// Agent fixture has already been uninstalled, perhaps by
-				// an explicit call to fixture.Uninstall, so nothing needs
-				// to be done here.
-				return
-			}
-			require.NoErrorf(f.t, err, "uninstalling agent failed. Output: %q", out)
+			f.t.Logf("ignoring AGENT_KEEP_INSTALLED=true as test succeeded, " +
+				"keeping the agent installed will jeperdise other tests")
+		}
+
+		out, err := f.Uninstall(ctx, &UninstallOpts{Force: true, UninstallToken: f.uninstallToken})
+		f.setClient(nil)
+		if err != nil &&
+			(errors.Is(err, ErrNotInstalled) ||
+				strings.Contains(
+					err.Error(),
+					"elastic-agent: no such file or directory")) {
+			f.t.Logf("fixture.Install Cleanup: agent was already uninstalled, skipping uninstall")
+			// Agent fixture has already been uninstalled, perhaps by
+			// an explicit call to fixture.Uninstall, so nothing needs
+			// to be done here.
+			return
 		}
+		require.NoErrorf(f.t, err, "uninstalling agent failed. Output: %q", out)
 	})
 
 	return out, nil
@@ -229,7 +235,7 @@ func (f *Fixture) collectDiagnostics() {
 	}
 }
 
-func collectDiag() bool {
+func collectDiagFlag() bool {
 	// failure reports false (ignore error)
 	v, _ := strconv.ParseBool(os.Getenv("AGENT_COLLECT_DIAG"))
 	return v