-
Notifications
You must be signed in to change notification settings - Fork 74
[test] Separate out the network tests #351
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
[test] Separate out the network tests #351
Conversation
|
/approve cancel |
ea06752 to
cf470a0
Compare
|
/retitle [test] Separate out the network tests |
mansikulkarni96
left a comment
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.
Thanks for working on this @aravindhp , PTAL at my comments.
cf470a0 to
090ec11
Compare
test/e2e/metrics_test.go
Outdated
| testCtx, err := NewTestContext() | ||
| require.NoError(t, err) | ||
| require.NoError(t, testCtx.createNamespace(testCtx.workloadNamespace), "error creating test namespace") |
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.
Why is this change being made? Is it because the metrics tests now run before the networking tests when run in CI? This will cause issues if the network tests are run independently. Instead the workload namespace creation/deletion can be handled in the common TestWMCO function.
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.
Is it because the metrics tests now run before the networking tests when run in CI?
Yes
Instead the workload namespace creation/deletion can be handled in the common TestWMCO function.
Moving the creation is not straightforward as testCtx.createNamespace would need to become testCtx.EnsureNamespace. Given how rarely we run the network tests independently I decided to put this off. But I can make the change as part of this PR.
090ec11 to
cc86f7b
Compare
|
|
||
| testCtx, err := NewTestContext() | ||
| require.NoError(t, err) | ||
| require.NoError(t, testCtx.ensureNamespace(testCtx.workloadNamespace), "error creating test namespace") |
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.
Doesnt this need to be cleaned up?
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.
That happens in the deletion tests
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sebsoto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
Move the network tests out from the creation tests. This is in preparation for separating out the upgrade and reconfiguration tests into a separate CI job.
cc86f7b to
c2f79b5
Compare
|
/lgtm |
|
/hold cancel |
|
/test vsphere-e2e-operator |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
14 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold until openshift/installer#4779 is merged. |
|
/hold cancel openshift/installer#4779 has merged |
|
/retest |
1 similar comment
|
/retest |
|
/cherry-pick release-4.7 |
|
/cherry-pick community-4.7 |
|
@aravindhp: new pull request created: #358 DetailsIn 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. |
|
@aravindhp: new pull request created: #359 DetailsIn 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. |
[OWNERS] Remove @selansen and add @aravindhp
Move the network tests out from the creation tests. This is in preparation for separating out the upgrade and reconfiguration tests into a separate CI job.