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

Improve pkg/controller/revision/revision.go coverage #1150

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

Improve pkg/controller/revision/revision.go coverage #1150

mattmoor opened this issue Jun 12, 2018 · 3 comments
Assignees
Labels
area/API API objects and controllers area/build Build topics specifically related to Knative kind/feature Well-understood/specified features, ready for coding.

Comments

@mattmoor
Copy link
Member

Move these to a helpers.go, and coverage in helpers_test.go:

  • getDeploymentProgressCondition could use focused unit testing,
  • getRevisionLastTransitionTime could use focused unit testing,
  • getBuildDoneCondition could use focused unit testing,
  • isBuildDone could use focused unit testing

addDeploymentProgressEvent needs coverage for most of its body.

removeFinalizers needs coverage for most of its body.

The logger.Errorf before many return err amplifies the lack of coverage along error paths. The runtime.HandleError pattern is similar, but less pronounced (probably not worth a helper).

This path seems easy to cover in addEndpointsEvent:

if revisionAge < serviceTimeoutDuration {
    return
}
@google-prow-robot google-prow-robot added area/API API objects and controllers area/build Build topics specifically related to Knative kind/feature Well-understood/specified features, ready for coding. labels Jun 12, 2018
@mattmoor mattmoor changed the title Improve pkg/controller/revision/revision.go converage Improve pkg/controller/revision/revision.go coverage Jun 12, 2018
google-prow-robot pushed a commit that referenced this issue Jun 12, 2018
This moves a bunch of small standalone helper functions into a separate file in our revision controller package and adds unit testing of each of them outside of the context of the controller itself.

Progress toward: #1150
@mattmoor
Copy link
Member Author

Once #1166 lands, I think the biggest coverage gap is removeFinalizers, at which point it is likely safe to resolve this.

@mattmoor
Copy link
Member Author

removeFinalizers looks unused, so I'm just removing it: #1173

google-prow-robot pushed a commit that referenced this issue Jun 12, 2018
The manner in which we were looking up the controlling Revision on Deployment events was relying on (an incorrect) naming convention where `deployment.Name == revision.Name`.  In reality, `deployment.Name == revision.Name + "-deployment"`, but relying on this for looking up the controlling object is an antipattern.

Instead, we should utilize `metav1.GetControllerOf` to determine the controlling `OwnerReference` and use this name to lookup the owning `Revision.

This fixes the local issue, but I've opened #1165 to track a broader triage of this.

Progress toward: #1150
@mattmoor
Copy link
Member Author

We should be in reasonable shape here.

nak3 pushed a commit to nak3/serving that referenced this issue Jul 4, 2022
skonto added a commit to skonto/serving that referenced this issue Jul 12, 2022
skonto added a commit to skonto/serving that referenced this issue Jul 14, 2022
* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>
skonto added a commit to skonto/serving that referenced this issue Jul 14, 2022
skonto added a commit to skonto/serving that referenced this issue Jul 27, 2022
* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>
skonto added a commit to skonto/serving that referenced this issue Jul 27, 2022
* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>
skonto added a commit to skonto/serving that referenced this issue Jul 27, 2022
* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>
skonto added a commit to skonto/serving that referenced this issue Jul 29, 2022
* [RELEASE-1.4] Inject images, generate manifests (knative#1169)

* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>

* fixes for 1.5

* Kourier image injection (knative#1173)

* Revert temoprary branch for image injection (knative#1186)

Co-authored-by: Kenjiro Nakayama <[email protected]>
nak3 added a commit to nak3/serving that referenced this issue Aug 3, 2022
* [RELEASE-1.4] Inject images, generate manifests (knative#1169)

* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>

* fixes for main

* Kourier image injection (knative#1173)

* Revert temoprary branch for image injection (knative#1186)

* add pdb fix

* Revert "add pdb fix"

This reverts commit 1790632.

Co-authored-by: Kenjiro Nakayama <[email protected]>
mgencur pushed a commit to mgencur/serving-1 that referenced this issue Sep 6, 2022
* [RELEASE-1.4] Inject images, generate manifests (knative#1169)

* inject images, generate manifests (knative#1150)

* Revert temoprary branch for image injection (knative#1159)

Co-authored-by: Kenjiro Nakayama <[email protected]>

* fixes for 1.6

* Kourier image injection (knative#1173)

* Revert temoprary branch for image injection (knative#1186)

Co-authored-by: Kenjiro Nakayama <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers area/build Build topics specifically related to Knative kind/feature Well-understood/specified features, ready for coding.
Projects
None yet
Development

No branches or pull requests

2 participants