-
Notifications
You must be signed in to change notification settings - Fork 40
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
OCPVE-619: Refactor E2E Tests to be context aware #378
OCPVE-619: Refactor E2E Tests to be context aware #378
Conversation
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
9acc2d5
to
92477b2
Compare
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
45d76ef
to
36a23a4
Compare
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
36a23a4
to
b9a4145
Compare
@jakobmoellerdev: This pull request references OCPVE-619 which is a valid jira issue. In response to this:
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. |
/cc @suleymanakbas91 |
b9a4145
to
257b6b1
Compare
controllers/topolvm_snapshotclass.go
Outdated
r.Log.Info("topolvm volume snapshot class is deleted", "VolumeSnapshotClass", vscName) | ||
return nil | ||
} | ||
if runtime.IsNotRegisteredError(err) || meta.IsNoMatchError(err) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we have the same check in ensureCreated
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are completely right. This didnt get catched because a failure will actually not change the LVMCluster to failed it will just log "failed to reconcile". This has to be reflected properly in the status, but that will be tackled with https://issues.redhat.com/browse/OCPVE-627 when we revisit the resource creation / status checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I anyhow added the check now also to create obviously
@@ -126,82 +127,91 @@ func lvmClusterTest() { | |||
Describe("Filesystem Type", Serial, func() { | |||
|
|||
var clusterConfig *v1alpha1.LVMCluster | |||
ctx := context.Background() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few more context.Background() calls left in setupPodAndPVC
and cleanupPVCAndPod
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Actually these methods were never used (thats why I didnt catch it) so i simply removed them.
257b6b1
to
b831233
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jakobmoellerdev, suleymanakbas91 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 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #378 +/- ##
===========================================
+ Coverage 16.59% 56.52% +39.93%
===========================================
Files 24 26 +2
Lines 2061 2259 +198
===========================================
+ Hits 342 1277 +935
+ Misses 1693 897 -796
- Partials 26 85 +59
|
/test lvm-operator-e2e-aws |
1 similar comment
/test lvm-operator-e2e-aws |
/test lvm-operator-e2e-aws |
@jakobmoellerdev: all tests passed! Full PR test history. Your PR dashboard. 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. |
Also introduces flag to skip Snapshot Tests on clusters without Snapshot CRDs by default (Openshift Local).
With the given guide and tests, I can get the entire snapshot test suite to run locally in about 10 minutes Right now. The rest of the latency fixes has to come from a follow-up PR that investigates minDeviceAge (opened up OCPVE-622 for this). After some testing, I believe we could get the test runtime down to sub 5 minutes with some small optimizations.
Known Issues:
In PVC and ephemeral tests the cleanup is run as a test. This causes leftover resources when the test fails and the suite cannot cleanup properly. This would require a bigger rewrite to the tests to fix so Im leaving this out.
Also:
fmt.Errorf
HACKING.md
into the docs folder and combines it with the E2E guide