-
Notifications
You must be signed in to change notification settings - Fork 255
Fix E2E tests to use fresh package for each RunCount iteration#4422
Merged
efiacor merged 1 commit intokptdev:mainfrom Mar 5, 2026
Merged
Fix E2E tests to use fresh package for each RunCount iteration#4422efiacor merged 1 commit intokptdev:mainfrom
efiacor merged 1 commit intokptdev:mainfrom
Conversation
Contributor
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:
- The first iteration modifies the package (e.g., namespace from "default" to "ns1")
- The second iteration operates on the already-modified package and has nothing to change
- 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
cntparameter andif cnt == 0check fromcompareResult()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.
aravindtga
requested review from
efiacor,
kispaljr and
mozesl-nokia
as code owners
March 3, 2026 16:28
Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
RunCountloop for bothfn evalandfn render. - Update result comparison to validate output/results on every iteration by removing the
cntgate incompareResult().
Comments suppressed due to low confidence (3)
pkg/test/runner/runner.go:155
- Deferring
os.RemoveAll(tmpDir)inside theforloop delays cleanup untilrunFnEval()returns, so multiple iterations will accumulate temporary directories on disk. Prefer cleaning up at the end of each iteration (e.g., explicitos.RemoveAll(tmpDir)after the iteration completes, or wrap the loop body in an immediately-invoked function anddeferinside 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 untilrunFnRender()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
actualnow 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.,actualResultsandactualDiff), 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.
aravindtga
force-pushed
the
fix-e2e-package-iteration
branch
from
March 3, 2026 16:42
37d9ec3 to
7b02747
Compare
Contributor
|
Looks like we may need this "fix" for the failing test - https://kubernetes.slack.com/archives/C0155NSPJSZ/p1772529551009119 |
liamfallon
approved these changes
Mar 4, 2026
Signed-off-by: aravind.est
aravindtga
force-pushed
the
fix-e2e-package-iteration
branch
from
March 5, 2026 09:40
7b02747 to
bf419f8
Compare
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, whenfnErr != nilyoubreakout 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 adefered 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
runFnEvalreturns early whenKPT_E2E_UPDATE_EXPECTEDis 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.RemoveAllerror. 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
runFnRendercreates a per-iteration temp dir here, but cleanup only happens at the bottom of the loop. Any earlyreturnin the iteration (including theKPT_E2E_UPDATE_EXPECTEDpath) will leak the temp dir. Consider per-iterationdefered 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
origPkgPathis created and the package is copied into it, but the directory is never referenced afterward inrunFnRender. If it's no longer needed for comparisons, remove the extraos.Mkdir/copyDirto 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.RemoveAllreturn 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
runFnEvalnow creates a new temp dir each iteration, but cleanup happens at the bottom of the loop. Any earlyreturnin the iteration (e.g., copy/prepare/setup failures) will leak the temp dir. Consider structuring each iteration with guaranteed cleanup (per-iteration closure +deferwhen 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.
efiacor
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.