Skip to content
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

Jump to bottom

test: Add tests for csicommon/server #316

Merged
merged 1 commit into from
Jun 24, 2020
Merged

test: Add tests for csicommon/server #316

merged 1 commit into from
Jun 24, 2020

Conversation

animeshk08
Copy link
Contributor

@animeshk08 animeshk08 commented Jun 23, 2020 •
edited
Loading

Signed-off-by: Animesh Kumar animuz111@gmail.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation

/kind failing-test
/kind feature
/kind flake

/kind test
What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Requirements:

Special notes for your reviewer:

Release note:

test: Add tests for csicommon/server

@k8s-ci-robot k8s-ci-robot added kind/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 23, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @animeshk08. 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 23, 2020
@andyzhangx
Copy link
Member

/ok-to-test

@k8s-ci-robot 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 Jun 23, 2020
@animeshk08 animeshk08 force-pushed the server-test branch 3 times, most recently from ee20ad4 to cff45fe Compare June 23, 2020 14:43
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 23, 2020
@animeshk08
Copy link
Contributor Author

/test pull-azurefile-csi-driver-sanity

@animeshk08
test: Add unit tests related to server
b30a98b 
This commit adds unit tests for csicommon/server,
csicommon/utils and azurefile/azurefile

Signed-off-by: Animesh Kumar <animuz111@gmail.com>
@animeshk08
Copy link
Contributor Author

This was a very tricky UT to write.

  • As discussed the server runs continuously hence, the unit tests never terminated. To prevent this a new parameter testMode has been added. This is then used to create a go-routine to close the server in case the methods are run in the testMode. This is the simplest way to cover these lines without doing any major change in the source code. Some other alternatives could have been to expose the server out of the Run method even then racing condition was introduced.

  • To prevent the racing condition the logic for stopping the server is included within the method serve rather than including it inside specific unit-tests.

  • A lot of the lines still remain uncovered. These primarily include ones involving klog.fatalf statement. This statement makes the program to exit with status code 255. Thus the UT also exits. On doing some digging I was able to find a work around. This required creating a subprocess, thus even though the UT ran and assertions could be tested ut coverage could not be generated. I also looked through upstream Kubernetes repo and was not able to find any examples to cover such cases. Please let me know in case one is found.

@animeshk08
Copy link
Contributor Author

Partially fixes #258

@animeshk08
Copy link
Contributor Author

The PR is ready for review @andyzhangx. I have tested it locally and the server is closed only while running in testMode

@andyzhangx
Copy link
Member

thanks. @animeshk08 I think it's ok now for writing such UT for csicommon/server, dig into more details on point# 3 may not help a lot. Could you work on other UTs? thanks.

@ZeroMagic could you also help review whether there is issue? We may apply such unit tests for all the other CSI drivers.
cc @Sakuralbj

@ZeroMagic
Copy link
Member

It looks good to me

andyzhangx
andyzhangx reviewed Jun 24, 2020
View reviewed changes
pkg/csi-common/server.go
@@ -50,9 +49,11 @@ type nonBlockingGRPCServer struct {
server *grpc.Server
}

func (s *nonBlockingGRPCServer) Start(endpoint string, ids csi.IdentityServer, cs csi.ControllerServer, ns csi.NodeServer) {
//var TestBool = false
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove this in next PR, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Will do it.

andyzhangx
andyzhangx reviewed Jun 24, 2020
View reviewed changes
pkg/csi-common/server.go
s.wg.Done()
go func() {
// make sure Serve() is called

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove empty line in next PR.

andyzhangx
andyzhangx approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@animeshk08 pls make same change on smb csi driver, thanks.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020
andyzhangx
andyzhangx reviewed Jun 24, 2020
View reviewed changes
pkg/csi-common/server.go
@@ -17,21 +17,20 @@ limitations under the License.
package csicommon

import (
"github.com/container-storage-interface/spec/lib/go/csi"
Copy link
Member

Choose a reason for hiding this comment

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

nit: pls keep sequence of import packages, e.g. first basic package, then github packages.

@k8s-ci-robot k8s-ci-robot merged commit 3c9f965 into kubernetes-sigs:master Jun 24, 2020
@animeshk08 animeshk08 mentioned this pull request Jun 29, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyzhangx andyzhangx andyzhangx approved these changes

@ZeroMagic ZeroMagic Awaiting requested review from ZeroMagic

Assignees

@andyzhangx andyzhangx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@animeshk08 @k8s-ci-robot @andyzhangx @ZeroMagic