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 group snapshot controller unit tests #1118

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Add group snapshot controller unit tests #1118

merged 1 commit into from
Aug 28, 2024

Conversation

leonardoce
Copy link
Contributor

@leonardoce leonardoce commented Jul 17, 2024 •
edited
Loading

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

What this PR does / why we need it:

Adds unit tests for the group snapshot controller.

Which issue(s) this PR fixes:

Fixes #1082

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 17, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 17, 2024
@leonardoce leonardoce marked this pull request as ready for review July 18, 2024 09:33
@k8s-ci-robot 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 Jul 18, 2024
yati1998
yati1998 suggested changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@yati1998 yati1998 left a comment

Choose a reason for hiding this comment

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

I would suggest to add the test for create,delete, update in single file

@leonardoce
Copy link
Contributor Author

I would suggest to add the test for create,delete, update in single file

Thank you, @yati1998, for having a look.
I split the unit tests because that's what it was done for volume snapshots.
Do you think it would better for volume group snapshots to have a different approach?

xing-yang
xing-yang reviewed Aug 12, 2024
View reviewed changes
pkg/common-controller/groupsnapshot_create_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_create_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_create_test.go Show resolved Hide resolved
pkg/common-controller/groupsnapshot_delete_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_delete_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_delete_test.go Outdated
expectSuccess: true,
},
{
name: "6-3 - dynamic group snapshot have been deleted, retention policy set to delete - volume snapshot content will be deleted",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a test for pre-provisioned group snapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test classification was not precise, I marked the pre-provisioned ones in a clearer way.

pkg/common-controller/groupsnapshot_finalizer_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_finalizer_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_finalizer_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_update_test.go Outdated Show resolved Hide resolved
Madhu-1
Madhu-1 reviewed Aug 21, 2024
View reviewed changes
pkg/common-controller/framework_test.go Show resolved Hide resolved
pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
pkg/common-controller/groupsnapshot_create_test.go Outdated Show resolved Hide resolved
@leonardoce leonardoce force-pushed the vgs-unit-tests branch 2 times, most recently from c8d5e10 to 6cdab61 Compare August 26, 2024 09:03
Madhu-1
Madhu-1 reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

small nits, LGTM

pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
pkg/common-controller/framework_test.go Outdated Show resolved Hide resolved
@leonardoce
Copy link
Contributor Author

Thank you @Madhu-1!

@leonardoce
Copy link
Contributor Author

Done @xing-yang, and thanks for the advice.
I also added another test for pre-provisioned volume group snapshots, testing the mismatch between a pre-provisioned volume group snapshot and a dynamically-provisioned volume group snapshot content.

@xing-yang
Copy link
Collaborator

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leonardoce, xing-yang

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 Aug 28, 2024
@k8s-ci-robot k8s-ci-robot merged commit 0a1fa5b into kubernetes-csi:master Aug 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yati1998 yati1998 yati1998 requested changes

@carlory carlory Awaiting requested review from carlory

@sunnylovestiramisu sunnylovestiramisu Awaiting requested review from sunnylovestiramisu

@ggriffiths ggriffiths Awaiting requested review from ggriffiths

@mauriciopoppe mauriciopoppe Awaiting requested review from mauriciopoppe

@Madhu-1 Madhu-1 Awaiting requested review from Madhu-1

@xing-yang xing-yang Awaiting requested review from xing-yang

Assignees

@xing-yang xing-yang

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add unit tests for Volume Group Snapshot
5 participants
@leonardoce @k8s-ci-robot @xing-yang @Madhu-1 @yati1998