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.

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

[WIP] test refactoring #6544

Closed
wants to merge 3 commits into from
Closed

Conversation

odra
Copy link

@odra odra commented Oct 7, 2021

What this PR does / why we need it:

Fixes #6495

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 #6495

Special notes for your reviewer:

WIP - sending a partial PR to test sonarcloud report.

I will update this section as the PR work evolves.

Release note:

NONE

@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: no Indicates the PR's author has not DCO signed all their commits. size/S needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 7, 2021
@kubevirt-bot
Copy link
Contributor

Hi @odra. Thanks for your PR.

I'm waiting for a kubevirt 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.

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign enp0s3 after the PR has been reviewed.
You can assign the PR to them by writing /assign @enp0s3 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@vatsalparekh
Copy link
Contributor

/ok-to-test

@kubevirt-bot kubevirt-bot 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 Oct 8, 2021
@odra odra force-pushed the issue_6594-refactor-tests branch from e663f2f to f7d43bc Compare October 9, 2021 18:15
Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

There is one compilation problem.
Generally, it's best if you run make && make test before submitting PR to avoid these small issues.

@@ -38,6 +38,20 @@ import (
"kubevirt.io/kubevirt/tests/util"
)


func gen_chroot_args(args...string) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Please use camelCase for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this can be named better. What it does is it will execute the virt-chroot binary. It will then switch to the mount namespace of the host(PID 1) and execute the given command in this "context".

Copy link
Author

Choose a reason for hiding this comment

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

I renamed it to "createVirtChrootCMD" since it just creates the command string and does not really execute anything.

Copy link
Author

Choose a reason for hiding this comment

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

Also, do you think that function should be moved to tests/utils.go?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it doesn't execute the actual command. It just creates a command that does what I described. createVirtChrootCMD doesn't tell me much except we will execute virt-chroot. But virt-chroot has a lot of usecases.

I don't think it's necessary to move it as it is only used in this file.

@odra odra force-pushed the issue_6594-refactor-tests branch 2 times, most recently from 3cae533 to 3b81df7 Compare October 20, 2021 13:57
for _, c := range job.Status.Conditions {
switch c.Type {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2021
Signed-off-by: Leonardo Rossetti <lrossett@redhat.com>
Signed-off-by: Leonardo Rossetti <lrossett@redhat.com>
@kubevirt-bot
Copy link
Contributor

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

  • 76d923c test/io_utils.go refactor

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 added size/L and removed size/M needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 12, 2021
@kubevirt-bot
Copy link
Contributor

@odra: 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.21-operator 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.21-operator
pull-kubevirt-e2e-k8s-1.20-operator 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.20-operator
pull-kubevirt-e2e-k8s-1.20-sig-compute-nonroot 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.20-sig-compute-nonroot
pull-kubevirt-e2e-k8s-1.20-sig-compute 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.20-sig-compute
pull-kubevirt-e2e-k8s-1.22-sig-network 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.22-sig-network
pull-kubevirt-e2e-k8s-1.20-operator-nonroot 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.20-operator-nonroot
pull-kubevirt-e2e-windows2016 50d0a88 link true /test pull-kubevirt-e2e-windows2016
pull-kubevirt-e2e-k8s-1.22-sig-compute 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.22-sig-compute
pull-kubevirt-e2e-k8s-1.21-sig-compute 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.21-sig-compute
pull-kubevirt-e2e-k8s-1.22-sig-storage 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.22-sig-storage
pull-kubevirt-e2e-k8s-1.21-sig-storage 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.21-sig-storage
pull-kubevirt-e2e-k8s-1.20-sig-network 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.20-sig-network
pull-kubevirt-e2e-k8s-1.22-operator 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.22-operator
pull-kubevirt-e2e-k8s-1.21-sig-network 50d0a88 link true /test pull-kubevirt-e2e-k8s-1.21-sig-network
pull-kubevirt-e2e-kind-1.19-vgpu 50d0a88 link true /test pull-kubevirt-e2e-kind-1.19-vgpu
pull-kubevirt-e2e-kind-1.19-sriov 50d0a88 link true /test pull-kubevirt-e2e-kind-1.19-sriov
pull-kubevirt-check-unassigned-tests 50d0a88 link true /test pull-kubevirt-check-unassigned-tests
pull-kubevirt-check-tests-for-flakes 50d0a88 link false /test pull-kubevirt-check-tests-for-flakes

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
Copy link
Contributor

@odra: PR needs rebase.

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.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2021
@kubevirt-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@kubevirt-bot kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 30, 2022
@kubevirt-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

@kubevirt-bot kubevirt-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 29, 2022
@kubevirt-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

@kubevirt-bot
Copy link
Contributor

@kubevirt-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[outreachy] [good-first-issue] Refactor tests
4 participants