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.

Sign up for GitHub

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

fix infrastructure/containerd integration tests #120

Merged
merged 1 commit into from
Oct 7, 2021
Merged

fix infrastructure/containerd integration tests #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion infrastructure/containerd/image_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,16 @@ func TestImageService_Integration(t *testing.T) {
Expect(len(leases)).To(Equal(1))
Expect(leases[0].ID).To(Equal(expectedLeaseName), "expect lease with name %s to exists", expectedLeaseName)

inputGet.ImageName = "docker.io/linuxkit/kernel:5.4.129"
inputGet.ImageName = testImageKernel

err = imageSvc.Pull(ctx, inputGet)
Expect(err).NotTo(HaveOccurred())

err = client.ImageService().Delete(namespaceCtx, testImageKernel)
Expect(err).NotTo(HaveOccurred())

err = client.ImageService().Delete(namespaceCtx, testImageVolume)
Expect(err).NotTo(HaveOccurred())
}

func testCreateClient(t *testing.T) (*ctr.Client, context.Context) {
Expand Down
3 changes: 3 additions & 0 deletions infrastructure/containerd/repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ func TestMicroVMRepo_Integration_MultipleSave(t *testing.T) {
Expect(err).NotTo(HaveOccurred())
Expect(savedVM).NotTo(BeNil())
Expect(savedVM.Version).To(Equal(2))

err = repo.Delete(ctx, testVm)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these delete functions be in an AfterEach? In case the above lines fail, this will not be called so in case of a failure the repo isn't cleaned up. That might be okay, but that could influence the test coming after this?

What do you think? ginkgo provides a thing called BeforeEach and AfterEach for setup purposes. If you would not like to use that, maybe use a defer delete? What do you guys think? :) This is just a question. :D

Copy link
Member

Choose a reason for hiding this comment

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

I tried to stay away from Ginkgo initially but its worth reconsidering.....lets discuss in stand-up and revisit this if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case:
I was thinking about a cleanup section, started to do it with defer, than I realized it's more useful if we test delete all the time, does not hurt and we can be sure it works in all cases.

Ginko for the project:
At this stage, I don't know if ginko is something we want to add, I did not see any special use case for it yet. These are integration tests and it these tests we want to test the whole workflow with all return values.

Full personal opinion on ginko:
As a personal opinion, I don't like (hate would be a hash word) ginko, it does not affect my opinion on the topic (above), but I like defining functions separately with a well defined workflow. I don't like the idea of nested functions, it's not JavaScript, i rather not use Describe { Describe { Context { ... } }}. I think it's more like "i like go, but ruby and JavaScript are better so let's implement the logic here". So in summary, if I want to pull in a testing framework for my personal projects, ginko wouldn't even be on the list.

Expect(err).NotTo(HaveOccurred())
}

func makeSpec(name, ns string) *models.MicroVM {
Expand Down