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

Clean up Revision test code. #1219

Closed
mattmoor opened this issue Jun 14, 2018 · 3 comments
Closed

Clean up Revision test code. #1219

mattmoor opened this issue Jun 14, 2018 · 3 comments
Assignees
@mattmoor
Labels
area/API API objects and controllers kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@mattmoor
Copy link
Member

Refactor the Revision controller tests to use the pattern adopted in #1216.
@google-prow-robot google-prow-robot added area/API API objects and controllers kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 14, 2018
@mattmoor
Copy link
Member Author

In order to use the new testing style I believe all Get() operations must be through the Informer's Lister. This means we need to convert the ConfigMap Get for the fluentd ConfigMap. I'd avoided this because the Informer we're passed is filtered to our system namespace.

@mattmoor mattmoor mentioned this issue Jun 30, 2018
Merged
google-prow-robot pushed a commit that referenced this issue Jul 2, 2018
@mattmoor @google-prow-robot
Revision controller table testing. (#1437)
59377fa 
* Revision controller table testing.

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: #1219

* Hoist some of the boilerplate from the main loop of the table test.

* Generalize the controller table test loop.

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.

* Comment out a useful logging line that we probably don't want on by default.
@mattmoor mattmoor mentioned this issue Jul 5, 2018
Merged
@mattmoor mattmoor reopened this Jul 6, 2018
@mattmoor
Copy link
Member Author

mattmoor commented Jul 6, 2018

I think the main piece of this that's remaining is coverage of the VPA code, which is blocked on the VPA stuff switching to informers/listers. Especially with #1501 available, the VPA stuff is definitely the most obvious gap in our coverage.

cc @josephburnett

@mattmoor
Copy link
Member Author

Going to close this in favor of move the VPA stuff into the autoscaling controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mattmoor mattmoor

Labels
area/API API objects and controllers kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mattmoor @google-prow-robot