Skip to content

Fixed TestAssetsFromDir unit test failure on Windows #21742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixed TestAssetsFromDir unit test failure on Windows #21742

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8349f0
Fixed TestAssetsFromDir unit test failure in Windows
bobsira Oct 13, 2025
444f0c3
fixed TestAssetsFromDir unit test failure in Windows
bobsira Oct 29, 2025
f422a25
Fixed TestAssetsFromDir unit test failure in Windows
bobsira Oct 29, 2025
6e0f4b8
fixed TestAssetsFromDir unit test failure in windows
bobsira Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions pkg/minikube/machine/filesync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ func TestAssetsFromDir(t *testing.T) {
vmPath: "/",
},
}
var testDirs = make([]string, 0)

var testDirs []string
defer func() {
for _, testDir := range testDirs {
err := os.RemoveAll(testDir)
if err != nil {
if err := os.RemoveAll(testDir); err != nil {
t.Logf("got unexpected error removing test dir: %v", err)
}
}
Expand All @@ -98,7 +98,6 @@ func TestAssetsFromDir(t *testing.T) {
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
testDir := testutil.MakeTempDir(t)

testDirs = append(testDirs, testDir)
testFileBaseDir := filepath.Join(testDir, test.baseDir)
want := make(map[string]string)
Expand Down Expand Up @@ -128,9 +127,17 @@ func TestAssetsFromDir(t *testing.T) {
}

actualFiles, err := assetsFromDir(testFileBaseDir, test.vmPath, test.flatten)

t.Cleanup(func() {
Copy link
Contributor

@nirs nirs Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be called before calling assetsFromDir().

Copy link
Contributor Author

@bobsira bobsira Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify more on this because the current placement looks fine to me since:

  • assetsFromDir opens the files (via assets.NewFileAsset) and returns []assets.CopyableFile. We don’t have anything to close until you have that slice.
  • We register t.Cleanup immediately after obtaining actualFiles. That’s the earliest possible point we can close them reliably.
  • t.Cleanup runs after the subtest finishes (even if it fails), so descriptors are released in all cases.
  • Moving the cleanup registration “before” the call would be impossible (the slice doesn’t exist yet) unless we refactored assetsFromDir to accept a callback, which is unnecessary for a test.

Copy link
Contributor

@nirs nirs Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if assetsFromDir fails, we don't have anything to cleanup? Can return incomplete results and fail?

If it either return slice or fail without opening anything we are fine with current code. If it can open some files and fail, we can either clean up in assetsFromDir, or here by registring the cleanup before we call:

var actualFiles []Xxx
t.Cleanup(...)
actualFiles = ...

But it will probably better to cleanup in assetsFromDir() or a test helper using it. Test helpers should get a t *testing.T argument and handle cleanup and test failures internally so the test code is simpler.

Copy link
Contributor Author

@bobsira bobsira Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced a collectAssets(t, root, dest, flatten) test helper that:

  • Calls assetsFromDir once and immediately registers a t.Cleanup to close any FileAsset descriptors it opened (including partial slices if the walk fails).
  • Keeps error handling + cleanup localized so each subtest only sets up files and validates the mapping. This addresses the earlier concern about calling cleanup “early enough” without modifying production code. Moving cleanup into assetsFromDir would change its resource lifecycle semantics for other callers, so keeping it in a test helper preserves current behavior while preventing leaks on Windows. Let me know if you’d prefer an additional error-path test; happy to add one if we can deterministically trigger a walk failure.

for _, f := range actualFiles {
if cerr := f.Close(); cerr != nil {
t.Logf("warning: closing asset %s failed: %v", f.GetSourcePath(), cerr)
}
}
})

if err != nil {
t.Errorf("got unexpected error adding minikube dir assets: %v", err)
return
t.Fatalf("got unexpected error adding minikube dir assets: %v", err)
}

got := make(map[string]string)
Expand Down