Skip to content

Move the Configuration controller testing to use the new table-testing method.#1333

Merged
google-prow-robot merged 4 commits intoknative:masterfrom
mattmoor:config-testing
Jun 25, 2018
Merged

Move the Configuration controller testing to use the new table-testing method.#1333
google-prow-robot merged 4 commits intoknative:masterfrom
mattmoor:config-testing

Conversation

@mattmoor
Copy link
Copy Markdown
Member

This uses the same strategy as the Service controller, which is based on the OpenShift Ingress controller's tests. Backport a few fixes to the way we set up ObjectMeta and OwnerReferences in our Service tests uncovered bringing this up. Move the old test methods over to the queueing_test.go, which still uses some of them. This also changes the way we create Build names to more easily test it (mirrors what we do for Revision today).

The most notable difference from the Service controller testing is that this needs to watch for Actions across 2 clients, so I refactored the loop body a bit to support that. This should make it easier to copy to Route and Revision.

Fixes: #1220

mattmoor added 2 commits June 23, 2018 14:49
In anticipation of rewriting the bulk of the Configuration controllers tests, move the current helper methods to the file that will keep using them.
…g method.

This uses the same strategy as the Service controller, which is based on the OpenShift Ingress controller's tests.  Backport a few fixes to the way we set up ObjectMeta and OwnerReferences in our Service tests uncovered bringing this up.  This also changes the way we create Build names to more easily test it (mirrors what we do for Revision today).

The most notable difference from the Service controller testing is that this needs to watch for Actions across 2 clients, so I refactored the loop body a bit to support that.  This should make it easier to copy to Route and Revision.

Fixes: knative/serving#1220
@google-prow-robot google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 23, 2018
Comment thread pkg/controller/service/service_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this "child object metadata"? I'm a fan of the brief names but a function-level comment here would help :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

From the PR description / commit message:

This also changes the way we create Build names to more easily test it (mirrors what we do for Revision today).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tiniest nit: "builds"

Comment thread pkg/controller/service/service_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tiniest nit: "builds"

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/controller/configuration/configuration.go 80.4% 88.0% 7.6

*TestCoverage feature is being tested, do not rely on any info here yet

@mattmoor
Copy link
Copy Markdown
Member Author

/test pull-knative-serving-unit-tests

@jonjohnsonjr
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018
@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonjohnsonjr, mattmoor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@google-prow-robot google-prow-robot merged commit 9a1580e into knative:master Jun 25, 2018
@mattmoor mattmoor deleted the config-testing branch June 25, 2018 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean up Configuration test code.

5 participants