add sanity testing #21
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jacobwolfaws 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 |
k8s-ci-robot
added
approved
| LustreConfiguration: "DeploymentType=CACHE_1,PerUnitStorageThroughput=1000,MetadataConfiguration={StorageCapacity=2400}" | ||
| copyTagsToDataRepositoryAssociations: "true" | ||
| extraTags: "Tag1=Value1,Tag2=Value2" | ||
| extraTags: "Name=DynamicProvisioningDemo" |
There was a problem hiding this comment.
Nit: could you change this back?
| mountOptions: | ||
| - flock | ||
| persistentVolumeReclaimPolicy: Recycle | ||
| persistentVolumeReclaimPolicy: Retain |
There was a problem hiding this comment.
I know that this change is planned but I'm not sure if you wanted to include it here. If you do, could you please expand your original git message to include the context?
There was a problem hiding this comment.
I don't think it's big enough to be included in the git message, but I'll make a note of it in the PR
| mountOptions: | ||
| - flock | ||
| persistentVolumeReclaimPolicy: Retain | ||
| persistentVolumeReclaimPolicy: Delete |
There was a problem hiding this comment.
Same here for adding context
There was a problem hiding this comment.
Will change this back to retain
| testFunc func(t *testing.T) | ||
| }{ | ||
| { | ||
| //TODO: Expand test checks after adding more values in FileCache object |
There was a problem hiding this comment.
We should add the following tests:
- create w/ an S3 DRA
- create w/ an S3 DRA but there's a invalid path so the create fails
- create w/ NFS but it's missing DNS server IP addresses or Cache path
- CreateFileCacheWithContext return error
There was a problem hiding this comment.
Since this mocks the API call, I don't believe we can have these tests without adding additional checks in the functions themselves. I think we'd have to leave these for e2e testing. CreateFileCacheWithContext return error is also included in this PR
| return nil, status.Error(codes.AlreadyExists, err.Error()) | ||
| default: | ||
| return nil, status.Errorf(codes.Internal, "Could not create volume %q: %v") | ||
| return nil, status.Errorf(codes.Internal, "Could not create volume %q: %v", volName, err) |
| } | ||
|
|
||
| // canSafelySkipMountPointCheck mocks base method. | ||
| func (m *MockMounter) canSafelySkipMountPointCheck() bool { |
There was a problem hiding this comment.
I'm confused on the purpose of this method, do you mind clarifying?
There was a problem hiding this comment.
"Package mocks is a generated GoMock package." This was auto-generated using the hack/update-gomock. Not sure what the specific use-case is here but I think we should leave it
|
/lgtm |
k8s-ci-robot
added
the
lgtm
4 participants
Is this a bug fix or adding new feature?
testing
What is this PR about? / Why do we need it?
Adding sanity testing to the repository
Further update should be made to the FileCache return object for more validation, but that was out of scope for this change
What testing is done?
make testsAdditional Changes made:
RecycletoRetainin the example because Recycle is a deprecated keyword