Test reboots #170
Test reboots #170
Conversation
qinqon
requested review from
phoracek and
SchSeba
and removed request for
phoracek
qinqon
force-pushed
the
test_reboots
branch
2 times, most recently
from
faf424f to
a214102
Compare
|
@qinqon please rebase |
qinqon
force-pushed
the
test_reboots
branch
from
cfc58b7 to
a4bd61c
Compare
test/e2e/reboot_test.go
Outdated
| var _ = Describe("Nmstate network connnections persistency", func() { | ||
| var ( | ||
| vlan101 = nmstatev1alpha1.State(`interfaces: | ||
| interfaces: |
There was a problem hiding this comment.
Help me here please. Having tests focused on a single thing is important. However, our main concern is whether the default interface comes up with the same IP, otherwise we lose access to the host. Could we test that specifically?
There was a problem hiding this comment.
(I want a test that fails due to changed IP and forces us to fix that bug)
There was a problem hiding this comment.
Well this is just a test to ensure that network configuration is apply before kubernetes-nmstate is up and ready, but we can increase the reboot test with the issue we have found at this PR or a follow up PR whatever you see it's better.
test/e2e/reboot_test.go
Outdated
| Context("after rebooting a node with applied configuration", func() { | ||
| BeforeEach(func() { | ||
| By("Apply policy") | ||
| setDesiredStateWithPolicy("vlan-eth0.101", vlan101) |
There was a problem hiding this comment.
The test body verifies what was done in the BeforeEach section, with the result that
- the BeforeEach / AfterEach functions are less reusable (there will probably won't be another test in this context)
- the code is not readable
Would it make sense to get rid of BeforeEach / AfterEach and having all in the test body?
There was a problem hiding this comment.
I think you are right although is not very ginkgo way, what do you think @phoracek
There was a problem hiding this comment.
I don't agree that it is less readable. It is more lines, but it very explicitly splits the setup, the test and the teardown.
I don't mind keeping everything in the test body, but if you chose to do it, please do it in separate PR and keep it consistent across the whole project.
There was a problem hiding this comment.
BeforeEach / AfterEach are meant to contain setup / tear down code independent of the body of the test, while here we have the actual thing that the test is going to verify.
In order to understand what the test does you need to jump between the functions which it makes a bit less readable for my taste but again, it's not a big deal.
qinqon
force-pushed
the
test_reboots
branch
6 times, most recently
from
6d44d62 to
023b946
Compare
It will create a vlan interface with a policy and reboot the nodes preventing k8s from starting up, the vlan connection has to be there since is NM the one configured and it's persistent. The teardown code puts back the k8s configuration and ensure that k8s is at good shape. Closes nmstate#143 Signed-off-by: Quique Llorente <[email protected]>
kubevirt-bot
added
the
needs-rebase
|
@qinqon: 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. |
|
@qinqon: The following tests failed, say
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. |
[CARRY] Add midstream reviewers and approvers
No description provided.