test: enable csi-proxy.exe in workflow and enable windows unit tests #556

mayankshah1607 · 2020-09-29T11:57:41Z

Signed-off-by: Mayank Shah [email protected]

What type of PR is this?
/kind test

What this PR does / why we need it:
Enable windows unit tests and add CSI proxy mount to workflow
Which issue(s) this PR fixes:

Fixes #525

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

none

k8s-ci-robot · 2020-09-29T11:57:48Z

Hi @mayankshah1607. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andyzhangx · 2020-09-30T07:45:02Z

test/utils/testutil/testutil.go

+	return TestError{
+		LinuxError:   linuxError,
+		WindowsError: windowsError,
+		DarwinError:  darwinError,


is DarwinError necessary? I thought it's same as LinuxError.

I noticed in this PR that some test cases have slight variation in the error message across Darwin and Linux, so I thought it might make sense to have a separate field in this case

maybe not, let's remove it first. Usually macos test error should be same as linux.

and also pls squash commits, thanks.

mayankshah1607 · 2020-09-30T11:06:17Z

@andyzhangx I've made the changes. This PR is ready :)

andyzhangx · 2020-09-30T12:01:51Z

/ok-to-test

mayankshah1607 · 2020-09-30T12:12:05Z

/retest

mayankshah1607 · 2020-09-30T13:46:13Z

/retest

mayankshah1607 · 2020-09-30T15:01:56Z

/retest

pkg/azuredisk/nodeserver_test.go

mayankshah1607 · 2020-10-02T11:37:00Z

/retest

Signed-off-by: Mayank Shah <[email protected]>

andyzhangx · 2020-10-02T13:09:35Z

/retest

andyzhangx · 2020-10-02T14:17:07Z

@mayankshah1607 can you change this value to 180 * time.Second? I think 90s is not enough in Windows test. Also add a comment: // 90s is not enough in Windows disk detach test

azuredisk-csi-driver/test/e2e/testsuites/dynamically_provisioned_azuredisk_detach_tester.go

Line 92 in 18ae134

time.Sleep(90 * time.Second)

andyzhangx · 2020-10-03T13:36:48Z

pkg/azuredisk/nodeserver_test.go

+			desc: "Staging target missing ",
+			req:  csi.NodeUnstageVolumeRequest{VolumeId: "vol_1"},
+			expectedErr: testutil.TestError{
+				LinuxError:   status.Error(codes.InvalidArgument, "Staging target not provided"),


use a defaultError, similar to https://github.com/kubernetes-csi/csi-driver-smb/blob/36a4dbce197fa7dcb82b27d5548479c21b67c9e4/pkg/smb/nodeserver_test.go#L374, otherwise need to write twice for every test error

@andyzhangx The issue with that is that in some cases, Windows has a nil error. In that case, we need an explicit mention. For example -

azuredisk-csi-driver/pkg/azuredisk/nodeserver_test.go

Line 167 in 430a46a

WindowsError: nil, // different mount behaviour on windows,

if it's not an error case, we could skip that case on Windows, e.g. https://github.com/kubernetes-csi/csi-driver-smb/blob/36a4dbce197fa7dcb82b27d5548479c21b67c9e4/pkg/smb/nodeserver_test.go#L474

otherwise need to copy the error message for every case, that's a burden to write a test.

Makes sense. I have added the relevant changes. Thanks

andyzhangx · 2020-10-04T01:44:33Z

@mayankshah1607 can you change this value to 180 * time.Second? I think 90s is not enough in Windows test. Also add a comment: // 90s is not enough in Windows disk detach test

azuredisk-csi-driver/test/e2e/testsuites/dynamically_provisioned_azuredisk_detach_tester.go

Line 92 in 18ae134

time.Sleep(90 * time.Second)

already fixed by #563

andyzhangx · 2020-10-04T01:44:43Z

/test pull-azuredisk-csi-driver-e2e-migration-windows

Signed-off-by: Mayank Shah <[email protected]>

andyzhangx

/lgtm
/approve

k8s-ci-robot · 2020-10-05T00:25:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, mayankshah1607

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:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andyzhangx · 2020-10-05T00:25:17Z

/retest

andyzhangx · 2020-10-05T01:56:45Z

there is a random failure on Windows:

Failed to unmarshall layerchain json - invalid character '\x00' looking for beginning of value

andyzhangx · 2020-10-05T04:35:17Z

/retest

andyzhangx · 2020-10-05T06:07:58Z

/retest

k8s-ci-robot added kind/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 29, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 29, 2020

k8s-ci-robot requested review from andyzhangx and ZeroMagic September 29, 2020 11:57

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 29, 2020

mayankshah1607 force-pushed the windows-ut branch 2 times, most recently from a84855a to 631ce7e Compare September 29, 2020 13:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 29, 2020

mayankshah1607 force-pushed the windows-ut branch 4 times, most recently from 3c6ea79 to 99aae52 Compare September 30, 2020 06:54

mayankshah1607 changed the title ~~test: enable csi-proxy.exe in workflow and enable windows unit tests~~ [WIP] test: enable csi-proxy.exe in workflow and enable windows unit tests Sep 30, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2020

mayankshah1607 force-pushed the windows-ut branch from 8f8e316 to bf952ec Compare September 30, 2020 07:23

andyzhangx reviewed Sep 30, 2020

View reviewed changes

mayankshah1607 force-pushed the windows-ut branch from 03bb668 to 8eb38a8 Compare September 30, 2020 10:03

mayankshah1607 changed the title ~~[WIP] test: enable csi-proxy.exe in workflow and enable windows unit tests~~ test: enable csi-proxy.exe in workflow and enable windows unit tests Sep 30, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2020

mayankshah1607 force-pushed the windows-ut branch 2 times, most recently from 961d74c to 2dcd839 Compare September 30, 2020 12:30

mayankshah1607 mentioned this pull request Oct 1, 2020

test: Add support for running unit tests on Windows kubernetes-sigs/blob-csi-driver#253

Closed

andyzhangx reviewed Oct 1, 2020

View reviewed changes

pkg/azuredisk/nodeserver_test.go Outdated Show resolved Hide resolved

mayankshah1607 force-pushed the windows-ut branch from 2dcd839 to 14b2fa1 Compare October 2, 2020 11:27

mayankshah1607 force-pushed the windows-ut branch from 14b2fa1 to b93d973 Compare October 2, 2020 11:41

fix: Windows unit tests and enable csi-proxy.exe in workflow

430a46a

Signed-off-by: Mayank Shah <[email protected]>

mayankshah1607 force-pushed the windows-ut branch from b93d973 to 430a46a Compare October 2, 2020 11:45

andyzhangx reviewed Oct 3, 2020

View reviewed changes

mayankshah1607 force-pushed the windows-ut branch from 8c05ecc to 5b24dd3 Compare October 4, 2020 10:30

enable default error

79843a8

Signed-off-by: Mayank Shah <[email protected]>

mayankshah1607 force-pushed the windows-ut branch from 5b24dd3 to 79843a8 Compare October 4, 2020 10:33

andyzhangx approved these changes Oct 5, 2020

View reviewed changes

k8s-ci-robot assigned andyzhangx Oct 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2020

k8s-ci-robot merged commit eef5de2 into kubernetes-sigs:master Oct 5, 2020

mayankshah1607 mentioned this pull request Oct 27, 2020

REQUEST: New membership for mayankshah1607 kubernetes/org#2290

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: enable csi-proxy.exe in workflow and enable windows unit tests #556

test: enable csi-proxy.exe in workflow and enable windows unit tests #556

mayankshah1607 commented Sep 29, 2020 •

edited

Loading

k8s-ci-robot commented Sep 29, 2020

andyzhangx Sep 30, 2020

mayankshah1607 Sep 30, 2020

andyzhangx Sep 30, 2020

andyzhangx Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

andyzhangx commented Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

mayankshah1607 commented Oct 2, 2020

andyzhangx commented Oct 2, 2020

andyzhangx commented Oct 2, 2020 •

edited

Loading

andyzhangx Oct 3, 2020

mayankshah1607 Oct 4, 2020

andyzhangx Oct 4, 2020 •

edited

Loading

mayankshah1607 Oct 4, 2020 •

edited

Loading

andyzhangx commented Oct 4, 2020

andyzhangx commented Oct 4, 2020

andyzhangx left a comment

k8s-ci-robot commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

test: enable csi-proxy.exe in workflow and enable windows unit tests #556

test: enable csi-proxy.exe in workflow and enable windows unit tests #556

Conversation

mayankshah1607 commented Sep 29, 2020 • edited Loading

k8s-ci-robot commented Sep 29, 2020

andyzhangx Sep 30, 2020

Choose a reason for hiding this comment

mayankshah1607 Sep 30, 2020

Choose a reason for hiding this comment

andyzhangx Sep 30, 2020

Choose a reason for hiding this comment

andyzhangx Sep 30, 2020

Choose a reason for hiding this comment

mayankshah1607 commented Sep 30, 2020

andyzhangx commented Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

mayankshah1607 commented Sep 30, 2020

mayankshah1607 commented Oct 2, 2020

andyzhangx commented Oct 2, 2020

andyzhangx commented Oct 2, 2020 • edited Loading

andyzhangx Oct 3, 2020

Choose a reason for hiding this comment

mayankshah1607 Oct 4, 2020

Choose a reason for hiding this comment

andyzhangx Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

mayankshah1607 Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

andyzhangx commented Oct 4, 2020

andyzhangx commented Oct 4, 2020

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

andyzhangx commented Oct 5, 2020

mayankshah1607 commented Sep 29, 2020 •

edited

Loading

andyzhangx commented Oct 2, 2020 •

edited

Loading

andyzhangx Oct 4, 2020 •

edited

Loading

mayankshah1607 Oct 4, 2020 •

edited

Loading