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.
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
unittests: Fixes unit tests for Windows (part 2) #110399
Conversation
de03108
to
746688b
Compare
besides minor nits that @thockin captured: /lgtm |
/test pull-kubernetes-unit |
Hello @claudiubelu, my name is Marky Jackson and I am one of the k8s 1.25 bug triage shadow assigned to track this body of work. Is this still on track for 1.25? |
Yes. I've addressed the comments above, waiting for other reviews / for this to get merged. |
/test pull-kubernetes-e2e-gce-storage-slow |
8d59bbc
to
3bd36f4
Compare
FYI: This PR has minimal code changes, and it mostly contains test changes. This PR has been included in this PR #110981, in which we've ran the |
/test pull-kubernetes-e2e-capz-azure-disk |
/test pull-kubernetes-e2e-gce-storage-snapshot |
/assign @thockin |
d8f0489
to
38092cb
Compare
Currently, there are some unit tests that are failing on Windows due to various reasons: - volume mounting is a bit different on Windows: Mount will create the parent dirs and mklink at the volume path later (otherwise mklink will raise an error). - os.Chmod is not working as intended on Windows. - path.Dir() will always return "." on Windows, and filepath.Dir() should be used instead (which works correctly). - on Windows, you can't typically run binaries without extensions. If the file C:\\foo.bat exists, we can still run C:\\foo because Windows will append one of the supported file extensions ($env:PATHEXT) to it and run it. - Windows file permissions do not work the same way as the Linux ones. - /tmp directory being used, which might not exist on Windows. Instead, the OS-specific Temp directory should be used. Fixes a few other issues: - rbd.go: Return error in a case in which an error is encountered. This will prevent "rbd: failed to setup" and "rbd: successfully setup" log messages to be logged at the same time.
/test pull-kubernetes-e2e-gce-storage-snapshot |
if err := os.MkdirAll(dir, 0755); err != nil && !os.IsNotExist(err) { | ||
t.Errorf("failed to create dir [%s]: %v", dir, err) | ||
} | ||
|
||
// do a fake local mount | ||
diskMounter := util.NewSafeFormatAndMountFromHost(plug.GetPluginName(), plug.host) | ||
if err := diskMounter.FormatAndMount("/fake/device", dir, "testfs", nil); err != nil { | ||
device := "/fake/device" | ||
if goruntime.GOOS == "windows" { |
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.
@BenTheElder I know you have feelings about this. I am inclined to approve, but finding a more consistent pattern would be good.
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.
Thanks!
/lgtm
/approve
@@ -1146,7 +1146,7 @@ func (fc *FakeProvisioner) Provision(selectedNode *v1.Node, allowedTopologies [] | |||
return nil, fmt.Errorf("expected error") | |||
} | |||
} | |||
fullpath := fmt.Sprintf("/tmp/hostpath_pv/%s", uuid.NewUUID()) | |||
fullpath := fmt.Sprintf("/%s/hostpath_pv/%s", os.TempDir(), uuid.NewUUID()) |
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.
Why does this not need to use Join? Knowing that code tends to get copied, this should either be consistent or comment why not?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: claudiubelu, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind failing-test
/sig testing
/sig windows
What this PR does / why we need it:
Currently, there are some unit tests that are failing on Windows due to various reasons:
os.Chmod
is not working as intended on Windows.path.Dir()
will always return"."
on Windows, andfilepath.Dir()
should be used instead (which works correctly).C:\\foo.bat
exists, we can still runC:\\foo
because Windows will append one of the supported file extensions ($env:PATHEXT
) to it and run it./tmp
directory being used, which might not exist on Windows. Instead, the OS-specific Temp directory should be used.Fixes a few other issues:
rbd.go
: Return error in a case in which an error is encountered. This will prevent"rbd: failed to setup"
and"rbd: successfully setup"
log messages to be logged at the same time.Which issue(s) this PR fixes:
Related: #51540
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: