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 1 commit into
base: master
Choose a base branch
from
Open

Fixed TestAssetsFromDir unit test failure on Windows #21742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 22 additions & 35 deletions pkg/minikube/machine/filesync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,9 @@ func TestAssetsFromDir(t *testing.T) {
relativePath string
expectedPath string
}{
{
relativePath: "/dir1/file1.txt",
expectedPath: vmpath.GuestAddonsDir,
},
{
relativePath: "/dir1/file2.txt",
expectedPath: vmpath.GuestAddonsDir,
},
{
relativePath: "/dir2/file1.txt",
expectedPath: vmpath.GuestAddonsDir,
},
{relativePath: "/dir1/file1.txt", expectedPath: vmpath.GuestAddonsDir},
{relativePath: "/dir1/file2.txt", expectedPath: vmpath.GuestAddonsDir},
{relativePath: "/dir2/file1.txt", expectedPath: vmpath.GuestAddonsDir},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to the fix?

Not adding unrelated changes will help to fix the windows issues quicker. We can do these formatting changes later after the windows tests are stable.

},
vmPath: vmpath.GuestAddonsDir,
},
Expand All @@ -69,27 +60,18 @@ func TestAssetsFromDir(t *testing.T) {
relativePath string
expectedPath string
}{
{
relativePath: "/dir1/file1.txt",
expectedPath: "/dir1",
},
{
relativePath: "/dir1/file2.txt",
expectedPath: "/dir1",
},
{
relativePath: "/dir2/file1.txt",
expectedPath: "/dir2",
},
{relativePath: "/dir1/file1.txt", expectedPath: "/dir1"},
{relativePath: "/dir1/file2.txt", expectedPath: "/dir1"},
{relativePath: "/dir2/file1.txt", expectedPath: "/dir2"},
},
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,18 +80,16 @@ 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)
for _, fileDef := range test.files {
err := func() error {
path := filepath.Join(testFileBaseDir, fileDef.relativePath)
err := os.MkdirAll(filepath.Dir(path), 0755)
want[path] = fileDef.expectedPath
if err != nil {
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
return err
}
want[path] = fileDef.expectedPath
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change related to the windows fix? Let's focus on one change in this commit. If you have more fixed let's do them later.

I'm also not sure this is correct, previous intentionally added the path before checking for errors.


file, err := os.Create(path)
if err != nil {
Expand All @@ -122,17 +102,24 @@ func TestAssetsFromDir(t *testing.T) {
return err
}()
if err != nil {
t.Errorf("unable to create file on fs: %v", err)
return
t.Fatalf("unable to create file on fs: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good change but related to the windows fix.

}
}

actualFiles, err := assetsFromDir(testFileBaseDir, test.vmPath, test.flatten)
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

It this test fail here, the cleanup will not run since we call it only after this failure.

}

// Ensure file descriptors opened by assets.NewFileAsset are released (critical on Windows).
t.Cleanup(func() {
Copy link
Contributor

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().

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

got := make(map[string]string)
for _, actualFile := range actualFiles {
got[actualFile.GetSourcePath()] = actualFile.GetTargetDir()
Expand Down
Loading