Revision controller table testing. #1437
Conversation
google-prow-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor 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 |
google-prow-robot
added
approved
|
/lgtm |
|
/lgtm cancel |
google-prow-robot
added
size/XL
|
/retest |
google-prow-robot
added
size/XXL
mattmoor
changed the title
google-prow-robot
removed
the
do-not-merge/work-in-progress
| if err := c.reconcileVPA(ctx, rev); err != nil { | ||
| logger.Error("Failed to create the vertical pod autoscaler for Deployment", zap.Error(err)) | ||
| return err | ||
| phases := []struct { |
There was a problem hiding this comment.
probably won't make much difference now, but if this list gets longer, using a factory function could make this a lot more compact.
There was a problem hiding this comment.
TBH, this was to game code coverage a bit. I was getting sick of looking at 2 red lines per error path that I couldn't figure out how to cover, so I made it just 2 red lines in a loop...
| @@ -460,8 +460,8 @@ func (c *Controller) createDeployment(ctx context.Context, rev *v1alpha1.Revisio | |||
| if rev.Spec.ServingState == v1alpha1.RevisionServingStateReserve { | |||
| replicaCount = 0 | |||
| } | |||
| deployment := MakeServingDeployment(rev, c.getLoggingConfig(), c.getNetworkConfig(), c.getObservabilityConfig(), | |||
| c.getAutoscalerConfig(), c.controllerConfig, replicaCount) | |||
| deployment := MakeServingDeployment(rev, c.getLoggingConfig(), c.getNetworkConfig(), | |||
There was a problem hiding this comment.
time to make a struct to carry these configs?
There was a problem hiding this comment.
This just wraps the line differently, so I'm inclined to punt, but I may need to consider such a change to avoid dependency cycles as I move the controllers towards something like: #1443
| testQueueImage = "queueImage" | ||
| ) | ||
|
|
||
| func getTestRevision() *v1alpha1.Revision { |
There was a problem hiding this comment.
this getTestRevision() is the same from test to test. may be this should be in a variable testRevision (similar to testNamespace and testQueueImage)
There was a problem hiding this comment.
This is just moving from revision_test.go, and I think I'd be more comfortable leaving it alone in this change.
I'm also unsure about such a change because this couldn't be const, and I think it makes it too easy for one test to mutate a var in a way that another test becomes sensitive to.
There was a problem hiding this comment.
The motivation for moving things from revistion_test.go btw is to enable me to git rm revision_test.go to assess where our coverage is without any of the tests in that file :)
| } | ||
| } | ||
|
|
||
| func getTestControllerConfig() *ControllerConfig { |
There was a problem hiding this comment.
extract to file variable testControllerConfig ?
There was a problem hiding this comment.
See my prior comment about just moving things around and concerns about mutations that effect other tests.
There was a problem hiding this comment.
e.g. TestNoAutoscalerImageCreatesNoAutoscalers in revision_test.go clears the AutoscalerImage to test disabling single-tenant auto-scaling.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Default the FooLister fields to reduce boilerplate | ||
| rl := tt.fields.r |
There was a problem hiding this comment.
extract all these into one function so we can do
r1, b1, ... := fill_default(...) ?
or fill() could fill the struct itself
There was a problem hiding this comment.
Also, I worry that making the test code generic will add complexity.
For example, it may be easier to miss an erroneous continue here, and the tests will all passed
| client := fakeclientset.NewSimpleClientset(objs...) | ||
| buildClient := fakebuildclientset.NewSimpleClientset(buildObjs...) | ||
| // Set up our Controller from the fakes. | ||
| c := &Controller{ |
There was a problem hiding this comment.
could extract to a method that takes in tt.fields?
There was a problem hiding this comment.
(as above)
I think what I'm going to expose is a TableTest method in pkg/controller/testing that takes:
- A list of this table type (turned into a proper exported struct)
- A
func(controller.Options) controller.Interfacethat instantiates the controller given acontroller.Options(which will have all our fakes).
I think then the rest of the loop becomes generic boilerplate. :)
There was a problem hiding this comment.
Yes, I think refactoring into a library will be better. May be we could even add tests for them :P
| ) | ||
|
|
||
| // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. | ||
| func TestReconcile(t *testing.T) { |
There was a problem hiding this comment.
is it possible to break this function into multiple by grouping tests with similar characteristic?
that may make us robust in case we have an error in the test logic -- such may be localized to one test function and not all of them?
There was a problem hiding this comment.
So I did a few things to reduce the complexity of this loop, which I'm now thinking belongs in pkg/controller/testing. PTAL at the improvements and I'll follow up with a more generic refactoring.
| client := fakeclientset.NewSimpleClientset(objs...) | ||
| buildClient := fakebuildclientset.NewSimpleClientset(buildObjs...) | ||
| // Set up our Controller from the fakes. | ||
| c := &Controller{ |
There was a problem hiding this comment.
Yes, I think refactoring into a library will be better. May be we could even add tests for them :P
| // Now check that the Reconcile had the desired effects. | ||
| expectedNamespace, _, _ := cache.SplitMetaNamespaceKey(tt.key) | ||
|
|
||
| c.WorkQueue.ShutDown() |
There was a problem hiding this comment.
can we extract c.WorkQueue -> hasQueue into its own function?
There was a problem hiding this comment.
I created drainWorkQueue in the generalized version.
| t.Errorf("unexpected queue (-want +got): %s", diff) | ||
| } | ||
|
|
||
| var createActions []clientgotesting.CreateAction |
There was a problem hiding this comment.
can we please extract this to
creates, updates, deletes := extractActions(...)
|
Oops, with the |
This sets up the skeleton for writing table tests for the Revision controller, and adds a number of tests for it. The bulk of the Revision controller behavior is now covered by the table-based test. The notable gaps in coverage where `revision_test.go` still meaningfully increases coverage are the fluentd and VPA paths, which are flag guarded. Covering these gaps is possible via this method, but they must be switched to `.Get()` through an informer lister that we can fake and the flag to enable this functionality passed. Another notable difference from `revision_test.go` is that the table test doesn't make the same effort to test things like label propagation, which IMO should be done through more focused unit testing of the utilities that Reconcile builds upon instead of through Reconcile itself. I believe this is far enough along that we should focus on adding new testing here instead of `revision_test.go` whenever possible. Related: knative/serving#1219
This moves the implementation of the table test the controllers are moving towards into a common location. Since the loop is already designed to be fairly generic, this is a relatively straightforward move with no appreciable change in the logic from what was in Revision's `table_test.go`. This does effectively backport some of the improvements made to the core logic to Service and Configuration.
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
/test pull-knative-serving-integration-tests |
|
/lgtm |
This sets up the skeleton for writing table tests for the Revision controller, and adds a number of tests for it. The bulk of the Revision controller behavior is now covered by the table-based test. The notable gaps in coverage where
revision_test.gostill meaningfully increases coverage are the fluentd and VPA paths, which are flag guarded. Covering these gaps is possible via this method, but they must be switched to.Get()through an informer lister that we can fake and the flag to enable this functionality passed.Another notable difference from
revision_test.gois that the table test doesn't make the same effort to test things like label propagation, which IMO should be done through more focused unit testing of the utilities that Reconcile builds upon instead of through Reconcile itself.I believe this is far enough along that we should focus on adding new testing here instead of
revision_test.gowhenever possible.Progress on: #1219