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

Test for breaking RunPodSandbox by CNI failure #672

Merged
merged 1 commit into from
May 15, 2018
Merged

Conversation

jellonek
Copy link
Contributor

@jellonek jellonek commented May 9, 2018

This change is Reviewable

@ivan4th
Copy link
Contributor

ivan4th commented May 10, 2018

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


pkg/manager/runtime_test.go, line 347 at r1 (raw file):

}

func (tst *virtletCRITester) expectFailingPodSandbox(config *kubeapi.PodSandboxConfig) {

expectSmth is naming is used for mock methods that set expectations but don't do anything else I think. Maybe runPodSandboxAndExpectError?


pkg/manager/runtime_test.go, line 350 at r1 (raw file):

	_, err := tst.invoke("RunPodSandbox", &kubeapi.RunPodSandboxRequest{Config: config}, false)
	if err == nil {
		tst.t.Errorf("expected error from RunPodSanbox but received success")

"didn't get an expected error from RunPodSandbox"


pkg/manager/runtime_test.go, line 501 at r1 (raw file):

	sandboxes := criapi.GetSandboxes(1)
	sandboxes[0].Metadata.Uid = "should-fail-cni"
	tst.expectFailingPodSandbox(sandboxes[0])

Does this call produce any output if you add tst.verify() at the end?


Comments from Reviewable

@ivan4th
Copy link
Contributor

ivan4th commented May 10, 2018

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.


pkg/manager/runtime_test.go, line 501 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

Does this call produce any output if you add tst.verify() at the end?

Probably tst.verify() should be called in any case to record FDManager calls etc.


Comments from Reviewable

@jellonek
Copy link
Contributor Author

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.


pkg/manager/runtime_test.go, line 501 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

Probably tst.verify() should be called in any case to record FDManager calls etc.

Yes, as there is used recorder .verify() should be called at the end.


Comments from Reviewable

@jellonek
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


pkg/manager/runtime_test.go, line 347 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

expectSmth is naming is used for mock methods that set expectations but don't do anything else I think. Maybe runPodSandboxAndExpectError?

Done.


pkg/manager/runtime_test.go, line 350 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

"didn't get an expected error from RunPodSandbox"

Done.


pkg/manager/runtime_test.go, line 501 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

Yes, as there is used recorder .verify() should be called at the end.

Done.


Comments from Reviewable

@ivan4th
Copy link
Contributor

ivan4th commented May 14, 2018

:lgtm:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@lukaszo
Copy link
Contributor

lukaszo commented May 15, 2018

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@pigmej
Copy link
Contributor

pigmej commented May 15, 2018

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@pigmej pigmej merged commit 39af904 into master May 15, 2018
@pigmej pigmej deleted the jell/testcnifail branch May 15, 2018 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants