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

pkg/operator: Add unit tests #863

Merged
merged 1 commit into from Jun 19, 2019

Conversation

Projects
None yet
6 participants
@sgreene570
Copy link
Contributor

commented Jun 17, 2019

- What I did
Added a couple more unit tests to pkg/operator
- How to verify it
go test -v github.com/openshift/machine-config-operator/pkg/operator
- Description for the changelog

pkg/operator add unit tests

@kikisdeliveryservice
Copy link
Member

left a comment

Went through quickly and added some comments, overall wondering if the TestRenderAsset isn't already tested via some other unit or e2e test.

Def think the 2nd test shouldnt be a unit test and rather should be tested more holistically as part of e2e.

Show resolved Hide resolved pkg/operator/render_test.go Outdated
Show resolved Hide resolved pkg/operator/render_test.go Outdated
Show resolved Hide resolved pkg/operator/render_test.go Outdated
Show resolved Hide resolved pkg/operator/render_test.go Outdated
Show resolved Hide resolved pkg/operator/render_test.go Outdated
Show resolved Hide resolved pkg/operator/render_test.go
Show resolved Hide resolved pkg/operator/status_test.go Outdated

@sgreene570 sgreene570 force-pushed the sgreene570:steve-unit-tests branch 3 times, most recently from 2154cd3 to 74008cf Jun 18, 2019

RenderConfig: &renderConfig{
TargetNamespace: "testing-namespace",
Images: &Images{
MachineConfigController: "{MCC: PLACEHOLDER}"},

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jun 18, 2019

Contributor

Looks reasonable to me but I think we could improve this by adding a member to the test struct like in this case FindExpected: " namespace: testing-namespace" or so and have the test code assert that string exists in the output if FindExpected != nil?

This comment has been minimized.

Copy link
@sgreene570

sgreene570 Jun 18, 2019

Author Contributor

Sure, can do.

@ashcrow

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Thank you for this PR!

@sgreene570 sgreene570 force-pushed the sgreene570:steve-unit-tests branch from 74008cf to bd8d2cd Jun 18, 2019

@cgwalters

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

/approve
/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

commented Jun 19, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, sgreene570

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

@openshift-merge-robot openshift-merge-robot merged commit 2cd8ecb into openshift:master Jun 19, 2019

7 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/e2e-aws-op Job succeeded.
Details
ci/prow/e2e-aws-upgrade Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
ci/prow/verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.