Add test coverage of the Service controller code.#1216
Add test coverage of the Service controller code.#1216google-prow-robot merged 2 commits intoknative:masterfrom
Conversation
google-prow-robot
added
approved
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
| } | ||
| } | ||
|
|
||
| func Filter(resource string) func(obj interface{}) bool { |
There was a problem hiding this comment.
bit of a nit, but why not
func Filter(kind string)?
and maybe add comments on what this Filter can be used for
| Name: "incomplete", | ||
| Namespace: "foo", | ||
| }, | ||
| // No spec.{runLatest,pinned} => error |
|
|
||
| for i := range tt.wantCreates { | ||
| if i > len(actions)-1 { | ||
| t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) |
There was a problem hiding this comment.
Just curious why these are Fatals in some cases an errors in others.
There was a problem hiding this comment.
Copied from the OpenShift controller. Some are harder to continue from than others.
Unlike the other controllers, this follows the pattern of OpenShift's Ingress controller (thanks to @smarterclayton for the pointer). This enables us to have a largely table-driven approach to testing this, which makes adding tests surprisingly easy.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, vaikas-google The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
1 similar comment
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
| elascheme.AddToScheme(scheme.Scheme) | ||
| } | ||
|
|
||
| func PassSecond(f func(interface{})) func(interface{}, interface{}) { |
There was a problem hiding this comment.
The name doesn't make it obvious I should use it with the UpdateFunc property
There was a problem hiding this comment.
I added a detailed comment (like Filter) and renamed this to PassNew, since new is the argument being forwarded to the function. Open to better naming suggestions?
| } else if service.Spec.Pinned != nil { | ||
| c.Spec = service.Spec.Pinned.Configuration | ||
| } else { | ||
| return nil, errors.New("malformed Service: one of runLatest or pinned must be present.") |
There was a problem hiding this comment.
The controller bails out in this scenario with no error message visible to the user
I'd actually expect ServiceConditionConfigurationReady to become false with the reason being the configuration is not set in the Service's spec. This then causing the ServiceConditionReady to become false
There was a problem hiding this comment.
I'd actually prefer this to fail on admission though
There was a problem hiding this comment.
| // Assert that our fake implements the interface it is faking. | ||
| var _ listers.RevisionLister = (*RevisionLister)(nil) | ||
|
|
||
| func (r *RevisionLister) List(selector labels.Selector) (ret []*v1alpha1.Revision, err error) { |
There was a problem hiding this comment.
not using the named returns ret & err throughout this file - probably worth getting rid of it
Dropped named returns from listers.go Rename and document the UpdateFunc helper.
| elascheme.AddToScheme(scheme.Scheme) | ||
| } | ||
|
|
||
| func PassSecond(f func(interface{})) func(interface{}, interface{}) { |
There was a problem hiding this comment.
I added a detailed comment (like Filter) and renamed this to PassNew, since new is the argument being forwarded to the function. Open to better naming suggestions?
| // Assert that our fake implements the interface it is faking. | ||
| var _ listers.RevisionLister = (*RevisionLister)(nil) | ||
|
|
||
| func (r *RevisionLister) List(selector labels.Selector) (ret []*v1alpha1.Revision, err error) { |
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
/lgtm |
7 participants
Unlike the other controllers, this follows the pattern of OpenShift's Ingress controller (thanks to @smarterclayton for the pointer). This enables us to have a largely table-driven approach to testing this, which makes adding tests surprisingly easy.