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

Use fakeclient instead of reactor in vmclone #10908

Merged
merged 6 commits into from
Feb 9, 2024
Merged

Use fakeclient instead of reactor in vmclone #10908

merged 6 commits into from
Feb 9, 2024

Conversation

fossedihelm
Copy link
Contributor

@fossedihelm fossedihelm commented Dec 19, 2023 •
edited
Loading

What this PR does / why we need it:
Using fake client has a benefit to actually
assert on what exactly we can find after a cycle.
In this way we can check whether a restore or a
snapshot are gone or not.

This PR contains a refactoring of the whole vmclone test file.

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

Special notes for your reviewer:
In order to increase readability I split this work in logical steps.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

None

@kubevirt-bot
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

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Dec 19, 2023
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2024
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2024
@fossedihelm fossedihelm mentioned this pull request Jan 8, 2024
Closed
8 tasks
@Longchuanzheng
Copy link
Contributor

Longchuanzheng commented Jan 9, 2024 •
edited
Loading

/cc

ignore

@kubevirt-bot
Copy link
Contributor

@Longchuanzheng: GitHub didn't allow me to request PR reviews from the following users: Longchuanzheng.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc

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.

@fossedihelm fossedihelm mentioned this pull request Jan 22, 2024
Merged
8 tasks
@fossedihelm fossedihelm marked this pull request as ready for review January 24, 2024 13:52
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2024
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2024
@fossedihelm fossedihelm force-pushed the vmclone_refactoring branch 2 times, most recently from 5c73d8d to 8029240 Compare January 29, 2024 07:31
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@fossedihelm
Copy link
Contributor Author

/retest

xpivarc
xpivarc reviewed Feb 2, 2024
View reviewed changes
pkg/virt-controller/watch/clone/clone_test.go Outdated Show resolved Hide resolved
pkg/virt-controller/watch/clone/clone_test.go Outdated Show resolved Hide resolved
pkg/virt-controller/watch/clone/clone_test.go Outdated Show resolved Hide resolved
pkg/virt-controller/watch/clone/clone_test.go Outdated Show resolved Hide resolved
@fossedihelm
refactor(clone_test): replace multiple var definitions in a single va…
9801899 
…r block

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
refactor(clone_test): introduce some constants
78923c5 
Introduce `testCloneUID`, `testSnapshotName`, `testRestoreName`,
`testSnapshotContentName` constants.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
refactor(clone_test): merge multiple BeforeEach functions
16593f1 
There are two `BeforeEach` functions that can be merged into one.
Since the resulting function is big, let's split it into functions
for logical steps.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
refactor(clone_test): drop useless snapshotcontent reactor
e6ffcb9 
There is no reason to expect the snapshotContent api call,
since in the production code it does not happen.
This is a perfect example of why reactors can be bad sometimes.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
vmclone: improve readability in clone unit tests
f2799fc 
Instead of using plain `It` node, wrap logical and
functional contexts in `When` spec.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
vmclone: update clone unit tests to replace reactors
9d338b7 
Using fake client has a benefit to actually
assert on what exactly we can find after a cycle.
In this way we can check whether a restore or a
snapshot are gone or not.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@fossedihelm
Copy link
Contributor Author

Thank you @xpivarc! Comment addressed and rebased on main to be able to do Expect(err).To(MatchError(errors.IsNotFound, "errors.IsNotFound"), "<Resource> should not exists"))
PTAL

@xpivarc
Copy link
Member

xpivarc commented Feb 3, 2024

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2024
@xpivarc
Copy link
Member

xpivarc commented Feb 3, 2024

/cc @acardace @alicefr
Can you have a look at this?

@acardace
Copy link
Member

acardace commented Feb 8, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Feb 9, 2024 •
edited
Loading

@fossedihelm: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-k8s-1.29-sig-monitoring 9f58994 link true /test pull-kubevirt-e2e-k8s-1.29-sig-monitoring
pull-kubevirt-e2e-k8s-1.27-sig-compute 9d338b7 link unknown /test pull-kubevirt-e2e-k8s-1.27-sig-compute
pull-kubevirt-e2e-k8s-1.27-sig-network 9d338b7 link unknown /test pull-kubevirt-e2e-k8s-1.27-sig-network

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. I understand the commands that are listed here.

@kubevirt-bot kubevirt-bot merged commit fa7aa29 into kubevirt:main Feb 9, 2024
34 of 37 checks passed
This was referenced Feb 21, 2024
Merged
Merged
@Barakmor1
Copy link
Member

/cherry-pick release-1.2

@kubevirt-bot kubevirt-bot mentioned this pull request Feb 22, 2024
Closed
@kubevirt-bot
Copy link
Contributor

@Barakmor1: new pull request created: #11333

In response to this:

/cherry-pick release-1.2

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.

@fossedihelm fossedihelm mentioned this pull request Feb 26, 2024
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpivarc xpivarc xpivarc left review comments

@stu-gott stu-gott Awaiting requested review from stu-gott

@vladikr vladikr Awaiting requested review from vladikr

@enp0s3 enp0s3 Awaiting requested review from enp0s3

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@acardace acardace Awaiting requested review from acardace

@alicefr alicefr Awaiting requested review from alicefr

Assignees

@acardace acardace

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fossedihelm @kubevirt-bot @Longchuanzheng @xpivarc @acardace @kubevirt-commenter-bot @Barakmor1