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

Add integration test cases of file share #1037

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Add integration test cases of file share #1037

wants to merge 1 commit into from

Conversation

Shruthi-1MN
Copy link
Contributor

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

@Shruthi-1MN Shruthi-1MN force-pushed the IT-fileshare branch 5 times, most recently from 457d398 to c267bf3 Compare November 7, 2019 06:26
@Shruthi-1MN Shruthi-1MN changed the title [WIP] Add integration test cases of file share Add integration test cases of file share Nov 7, 2019
@Shruthi-1MN Shruthi-1MN force-pushed the IT-fileshare branch 6 times, most recently from 30b2191 to 1f249e8 Compare November 11, 2019 07:12
@codecov
Copy link

codecov bot commented Nov 11, 2019 •
edited
Loading

Codecov Report

Merging #1037 into development will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           development    #1037   +/-   ##
============================================
  Coverage        34.66%   34.66%           
============================================
  Files               97       97           
  Lines            17878    17878           
============================================
  Hits              6197     6197           
  Misses           10805    10805           
  Partials           876      876

kumarashit
kumarashit requested changes Nov 27, 2019
View reviewed changes
client/fileshare_test.go
@@ -299,7 +299,7 @@ func TestCreateFileShareAcl(t *testing.T) {
t.Error(err)
return
}

fileShareAcl.Status = "available"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should our test case include FielShareAcl.Statu = "unavailable" to for Create File Share ACL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only positive scenarios are covered in this cycle of execution. FileshareAcl.Status = "error" is there not "unavailable"

pkg/api/controllers/profile_test.go
@@ -493,15 +493,15 @@ func TestDeleteFileShareProfile(t *testing.T) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check if the new profile API (though it is just param changes), will have any effect on this?

pkg/api/controllers/profile_test.go
@@ -493,15 +493,15 @@ func TestDeleteFileShareProfile(t *testing.T) {

t.Run("Should return 200 if everything works well", func(t *testing.T) {
mockClient := new(dbtest.Client)
mockClient.On("GetProfile", c.NewAdminContext(), "2f9c0a04-66ef-11e7-ade2-43158893e017").Return(
mockClient.On("GetProfile", c.NewAdminContext(), "3f9c0a04-66ef-11e7-ade2-43158893e017").Return(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the ID change required for tests? Should not it be consistent or some generator which generates ID and that is persisted across the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are all compulsorily needed to mock functions GetProfile function, it was using block profile, So now i have changed to use file profiles.

pkg/controller/fileshare/filesharecontroller_test.go
@@ -163,7 +163,7 @@ func TestCreateFileShareAcl(t *testing.T) {
if err != nil {
t.Errorf("failed to create fileshare acl, err is %v\n", err)
}

result.Status = "available"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above, Should we have a test case for unavailable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only +ve scenarios covered for IT

test/integration/client_test.go
File share integration test cases
*/

func TestClientCreateFileProfile(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function should be split and one part should be made generic. Just pass the StorageType as Block, File or Object..It should create profile with the cutomeProperties. Caller will then do the match with the expected

test/integration/client_test.go
return
}

var expected = &SampleFileShares[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the expected value an array or struct or just a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its interface

client/fileshare_test.go
@@ -299,7 +299,7 @@ func TestCreateFileShareAcl(t *testing.T) {
t.Error(err)
return
}

fileShareAcl.Status = "available"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have a test case for unavailable test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

pkg/api/controllers/profile_test.go
@@ -493,15 +493,15 @@ func TestDeleteFileShareProfile(t *testing.T) {

t.Run("Should return 200 if everything works well", func(t *testing.T) {
mockClient := new(dbtest.Client)
mockClient.On("GetProfile", c.NewAdminContext(), "2f9c0a04-66ef-11e7-ade2-43158893e017").Return(
mockClient.On("GetProfile", c.NewAdminContext(), "3f9c0a04-66ef-11e7-ade2-43158893e017").Return(
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes are only for ID..Why tests should have changes related to ID..Shoud we have generator function for ID which can be used across the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data collection file is already there, we are reusing those. In this test case it was using block profile. So we have changed to file profile id

test/integration/client_test.go
@@ -546,3 +546,258 @@ func TestClientFailoverReplication(t *testing.T) {
t.Log("Disable volume replication not ready!")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not this file be separated for different features? like client_fs_test, client_block_test , client_relication_test....?
@joseph-v @Shruthi-1MN Can we please consider this?

testutils/collection/data.go
@@ -58,19 +58,19 @@ var (
SampleFileShareProfiles = []model.ProfileSpec{
{
BaseModel: &model.BaseModel{
Id: "1106b972-66ef-11e7-b172-db03f3689c9c",
Id: "2106b972-66ef-11e7-b172-db03f3689c9c",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above...Why should there be these chages for ID? Let's discuss it. Generator and persisted across tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above file profile has been created earlier it was using block profile

@Shruthi-1MN Shruthi-1MN force-pushed the IT-fileshare branch 5 times, most recently from cb0bdc5 to ec3c2b0 Compare November 27, 2019 09:23
@Shruthi-1MN Shruthi-1MN changed the base branch from master to development November 28, 2019 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThisIsClark ThisIsClark ThisIsClark left review comments

@kumarashit kumarashit kumarashit requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Shruthi-1MN @ThisIsClark @kumarashit