Fix E2E tests to use fresh package for each RunCount iteration

[aravindtga]

Problem

The E2E test framework runs kpt fn eval/render commands twice to verify consistent results. However, this will fails with the addition of function results to the Kptfile, because:

1. The first iteration modifies the package (e.g., namespace from "default" to "ns1")
2. The second iteration operates on the already-modified package and has nothing to change
3. Function results differ between iterations, making the comparison invalid

Solution

This PR modifies the test runner to create a fresh package copy for each RunCount iteration:

• Move package setup inside the loop: Each iteration now gets its own tmpDir with a fresh package copy
• Validate all iterations: Removed the cnt parameter and if cnt == 0 check from compareResult() so function results are validated for every iteration

Result

Each iteration now:

• Operates on an identical fresh package
• Produces consistent function results

This ensures the E2E tests correctly verify that kpt functions produce the same results when run on identical packages.

Testing

Existing E2E tests will now properly validate function results across all iterations.

[netlify]

✅ Deploy Preview for kptdocs ready!

Name	Link
🔨 Latest commit	bf419f8
🔍 Latest deploy log	https://app.netlify.com/projects/kptdocs/deploys/69a94faba5bdeb00081e66e0
😎 Deploy Preview	https://deploy-preview-4422--kptdocs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[Copilot]

Pull request overview

Updates the E2E test runner to ensure each RunCount iteration operates on a fresh, identical package copy so result comparisons remain valid when functions mutate the package.

Changes:

• Move temp-dir creation + package copy/prepare logic inside the RunCount loop for both fn eval and fn render.
• Update result comparison to validate output/results on every iteration by removing the cnt gate in compareResult().

Comments suppressed due to low confidence (3)

pkg/test/runner/runner.go:155

• Deferring os.RemoveAll(tmpDir) inside the for loop delays cleanup until runFnEval() returns, so multiple iterations will accumulate temporary directories on disk. Prefer cleaning up at the end of each iteration (e.g., explicit os.RemoveAll(tmpDir) after the iteration completes, or wrap the loop body in an immediately-invoked function and defer inside that function) so each iteration frees its temp dir promptly.

	for i := 0; i < r.testCase.Config.RunCount(); i++ {
		r.t.Logf("Running test against package %s, iteration %d \n", r.pkgName, i+1)
		tmpDir, err := os.MkdirTemp("", "krm-fn-e2e-*")
		if err != nil {
			return fmt.Errorf("failed to create temporary dir: %w", err)
		}
		pkgPath := filepath.Join(tmpDir, r.pkgName)

		if r.testCase.Config.Debug {
			fmt.Printf("Running test against package %s in dir %s \n", r.pkgName, pkgPath)
		}
		if !r.testCase.Config.Debug {
			// if debug is true, keep the test directory around for debugging
			defer os.RemoveAll(tmpDir)
		}

pkg/test/runner/runner.go:307

• Same issue as runFnEval(): defer os.RemoveAll(tmpDir) inside the loop will keep all per-iteration temp dirs around until runFnRender() returns. This can significantly increase disk usage and slow down longer runs; clean up per-iteration instead of deferring in the loop.

	for i := 0; i < r.testCase.Config.RunCount(); i++ {
		r.t.Logf("Running test against package %s, iteration %d \n", r.pkgName, i+1)
		tmpDir, err := os.MkdirTemp("", "kpt-pipeline-e2e-*")
		if err != nil {
			return fmt.Errorf("failed to create temporary dir: %w", err)
		}
		if r.testCase.Config.Debug {
			fmt.Printf("Running test against package %s in dir %s \n", r.pkgName, tmpDir)
		}
		if !r.testCase.Config.Debug {
			// if debug is true, keep the test directory around for debugging
			defer os.RemoveAll(tmpDir)
		}

pkg/test/runner/runner.go:474

• actual now holds two different concepts (actual results, then actual diff). This makes the function harder to follow and increases the chance of mistakes during future edits. Consider using distinct variables (e.g., actualResults and actualDiff), or keep the previous shadowing behavior (actualDiff, err := readActualDiff(...)) to preserve clarity.

	// compare results
	actual, err := readActualResults(resultsPath)
	if err != nil {
		return fmt.Errorf("failed to read actual results: %w", err)
	}

	actual = r.stripLines(actual, r.testCase.Config.ActualStripLines)

	diffOfResult, err := diffStrings(actual, expected.Results)
	if err != nil {
		return fmt.Errorf("error when run diff of results: %w: %s", err, diffOfResult)
	}
	if actual != expected.Results {
		return fmt.Errorf("actual results doesn't match expected\nActual\n===\n%s\nDiff of Results\n===\n%s",
			actual, diffOfResult)
	}

	// compare diff
	actual, err = readActualDiff(tmpPkgPath, r.initialCommit)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[efiacor]

Looks like we may need this "fix" for the failing test - https://kubernetes.slack.com/archives/C0155NSPJSZ/p1772529551009119

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

pkg/test/runner/runner.go:258

• In runFnEval, when fnErr != nil you break out of the loop before the per-iteration temp dir cleanup runs, so the temp directory will be leaked for expected-error cases. Move cleanup so it runs before breaking, or wrap the loop body in a per-iteration closure with a defered cleanup that runs even on early exits.

		// we passed result check, now we should break if the command error
		// is expected
		if fnErr != nil {
			break
		}

		err = r.runTearDownScript(pkgPath)
		if err != nil {
			return err
		}

		// cleanup temp directory after iteration
		if !r.testCase.Config.Debug {
			os.RemoveAll(tmpDir)
		}

pkg/test/runner/runner.go:237

• runFnEval returns early when KPT_E2E_UPDATE_EXPECTED is true, but the temp dir created for the iteration is not cleaned up (unless Debug is enabled). Consider ensuring cleanup happens on all return paths (e.g., defered cleanup inside a per-iteration closure).

		if strings.ToLower(os.Getenv(updateExpectedEnv)) == "true" {
			return r.updateExpected(pkgPath, resultsDir, filepath.Join(r.testCase.Path, expectedDir))
		}

pkg/test/runner/runner.go:258

• The per-iteration temp dir cleanup ignores the os.RemoveAll error. Even if you decide not to fail the test on cleanup errors, it would be helpful to at least log the error so leaked temp dirs can be diagnosed.

		// cleanup temp directory after iteration
		if !r.testCase.Config.Debug {
			os.RemoveAll(tmpDir)
		}

pkg/test/runner/runner.go:301

• runFnRender creates a per-iteration temp dir here, but cleanup only happens at the bottom of the loop. Any early return in the iteration (including the KPT_E2E_UPDATE_EXPECTED path) will leak the temp dir. Consider per-iteration defered cleanup (e.g., wrap the iteration body in a closure) so cleanup runs on all exit paths when Debug is false.

		tmpDir, err := os.MkdirTemp("", "kpt-pipeline-e2e-*")
		if err != nil {
			return fmt.Errorf("failed to create temporary dir: %w", err)
		}

pkg/test/runner/runner.go:331

• origPkgPath is created and the package is copied into it, but the directory is never referenced afterward in runFnRender. If it’s no longer needed for comparisons, remove the extra os.Mkdir/copyDir to avoid unnecessary I/O; if it is needed, consider adding a comment or using it in the comparison logic so the intent is clear.

		// create dir to store untouched pkg to compare against
		origPkgPath := filepath.Join(tmpDir, "original")
		err = os.Mkdir(origPkgPath, 0755)
		if err != nil {
			return fmt.Errorf("failed to create original dir %s: %w", origPkgPath, err)
		}

		var resultsDir, destDir string

		if r.IsFnResultExpected() {
			resultsDir = filepath.Join(tmpDir, "results")
		}

		if r.IsOutOfPlace() {
			destDir = filepath.Join(pkgPath, outDir)
		}

		// copy package to temp directory
		err = copyDir(r.testCase.Path, pkgPath)
		if err != nil {
			return fmt.Errorf("failed to copy package: %w", err)
		}
		err = copyDir(r.testCase.Path, origPkgPath)
		if err != nil {
			return fmt.Errorf("failed to copy package: %w", err)
		}

pkg/test/runner/runner.go:406

• The per-iteration cleanup ignores the os.RemoveAll return value. Even if cleanup failures shouldn't fail the test, consider logging the error to aid diagnosing leaked temp directories.

		// cleanup temp directory after iteration
		if !r.testCase.Config.Debug {
			os.RemoveAll(tmpDir)
		}

pkg/test/runner/runner.go:146

• runFnEval now creates a new temp dir each iteration, but cleanup happens at the bottom of the loop. Any early return in the iteration (e.g., copy/prepare/setup failures) will leak the temp dir. Consider structuring each iteration with guaranteed cleanup (per-iteration closure + defer when Debug is false).

		tmpDir, err := os.MkdirTemp("", "krm-fn-e2e-*")
		if err != nil {
			return fmt.Errorf("failed to create temporary dir: %w", err)
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.