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

Jump to bottom

Add lib unit tests #1994

Merged
merged 1 commit into from Mar 15, 2019
Merged

Add lib unit tests #1994

merged 1 commit into from Mar 15, 2019

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Jan 3, 2019 •
edited

This PR adds unit tests to the lib package. Beside that I added a config interface and fixed up some possible nil pointer dereferences I found during the test creation.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 3, 2019
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 3, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @saschagrunert. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@saschagrunert saschagrunert mentioned this pull request Jan 3, 2019
Closed
10 tasks
@openshift-ci-robot
Copy link

Hi @saschagrunert. Thanks for your PR.

I'm waiting for a kubernetes-sigs or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@TomSweeneyRedHat
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 3, 2019
@saschagrunert saschagrunert force-pushed the lib-unit-tests branch 3 times, most recently from 5d2ea87 to 2b4c3d5 Compare January 3, 2019 15:22
@saschagrunert saschagrunert mentioned this pull request Jan 4, 2019
Closed
@saschagrunert saschagrunert force-pushed the lib-unit-tests branch 5 times, most recently from 91e368d to 408ca80 Compare January 11, 2019 13:48
@saschagrunert saschagrunert force-pushed the lib-unit-tests branch 2 times, most recently from 8ccf4fc to ab30efb Compare February 4, 2019 12:01
@saschagrunert saschagrunert changed the title WIP: Add lib unit tests Add lib unit tests Feb 4, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2019
@openshift-bot
Copy link

/retest

@saschagrunert saschagrunert changed the title Add lib unit tests WIP: Add lib unit tests Mar 11, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2019
@saschagrunert
Copy link
Member Author

Mocking of the runtimeImpl is not possible any more on the latest master since the interface made private. @sboeuf I know this change was intended and from the current perspective it totally makes sense. Unfortunately only public interfaces can be mocked in go. How do we want to proceed here?

@sboeuf
Copy link

sboeuf commented Mar 11, 2019

@saschagrunert oh I completely missed this, I'm truly sorry. I didn't realize this was used for mocking (which is a great way to test IMO), that's why I simply made it private.
Now that there's a use case for this to be public, let's move it back to public then :)

/cc @mrunalp

@mrunalp
Copy link
Member

mrunalp commented Mar 11, 2019 via email

@saschagrunert
Copy link
Member Author

saschagrunert commented Mar 11, 2019 •
edited

@mrunalp @sboeuf Alright, I will make it public again. thanks for the fast response! :)

@sboeuf
Copy link

sboeuf commented Mar 11, 2019

Great, and sorry for the extra workload here...

@saschagrunert saschagrunert force-pushed the lib-unit-tests branch 2 times, most recently from 7671a4a to f85d660 Compare March 12, 2019 14:46
@saschagrunert saschagrunert changed the title WIP: Add lib unit tests Add lib unit tests Mar 12, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2019
@saschagrunert
Copy link
Member Author

/test e2e_fedora

umohnani8
umohnani8 reviewed Mar 13, 2019
View reviewed changes
lib/container_server_test.go
})

// TODO(sgrunert): uncomment when the following issue is resolved:
// https://github.com/kubernetes-sigs/cri-o/issues/1987
Copy link
Member

Choose a reason for hiding this comment

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

#1987 was fixed. This test can be uncommented now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I verified that it is not resolved yet, reopened the issue and prepared a fix within #2127

@umohnani8
Copy link
Member

Needs rebase @saschagrunert .
Changes LGTM

@saschagrunert
Add lib unit tests
109d370 
Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@saschagrunert saschagrunert mentioned this pull request Mar 14, 2019
Merged
@saschagrunert
Copy link
Member Author

/test e2e_rhel

@rhatdan
Copy link
Contributor

rhatdan commented Mar 15, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2019
@haircommander
Copy link
Member

@rhatdan @mrunalp @umohnani8 needs approved label
(is it possible to add approved label if an approver does /lgtm ?)

@rhatdan
Copy link
Contributor

rhatdan commented Mar 15, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, saschagrunert

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 697d022 into cri-o:master Mar 15, 2019
@saschagrunert saschagrunert deleted the lib-unit-tests branch March 15, 2019 13:07
@saschagrunert saschagrunert mentioned this pull request Mar 21, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umohnani8 umohnani8 umohnani8 left review comments

@mrunalp mrunalp Awaiting requested review from mrunalp

@runcom runcom Awaiting requested review from runcom

@giuseppe giuseppe Awaiting requested review from giuseppe

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@saschagrunert @k8s-ci-robot @openshift-ci-robot @TomSweeneyRedHat @openshift-bot @sboeuf @mrunalp @umohnani8 @rhatdan @haircommander