Skip to content

Adding missing UTs for Source reconcilers#2828

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
nachocano:eventtype_uts
Mar 24, 2020
Merged

Adding missing UTs for Source reconcilers#2828
knative-prow-robot merged 3 commits into
knative:masterfrom
nachocano:eventtype_uts

Conversation

@nachocano
Copy link
Copy Markdown
Contributor

@nachocano nachocano commented Mar 24, 2020

Proposed Changes

Release Note

NONE

Docs

/assign @n3wscott

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 24, 2020
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 24, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Mar 24, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2020
mattmoor
mattmoor reviewed Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

nachocano
nachocano commented Mar 24, 2020
View reviewed changes
Comment thread pkg/reconciler/source/crd/crd_test.go
},
},
//}, {
// TODO uncomment the following once we figure out why the eventtype informer is missing from the duck.controller.
Copy link
Copy Markdown
Contributor Author

@nachocano nachocano Mar 24, 2020

Choose a reason for hiding this comment

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

@n3wscott I'm having trouble with this one.
It seems that the context we pass to the Source duck controller doesn't have eventtype:

eventing/pkg/reconciler/source/crd/crd.go

Line 203 in 7dca1df

sd := sdc(sdctx, r.ogcmw)
, although I'm importing the fake...
I might be missing something, OR there is a bug in

eventing/pkg/reconciler/source/crd/crd.go

Line 201 in 7dca1df

sdctx, cancel := context.WithCancel(r.ogctx)
...
Any ideas?

Copy link
Copy Markdown
Contributor

@n3wscott n3wscott Mar 24, 2020

Choose a reason for hiding this comment

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

What is the error that is thrown?

Copy link
Copy Markdown
Contributor

@n3wscott n3wscott Mar 24, 2020

Choose a reason for hiding this comment

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

does it not have the kind EventType? I need more info to try to help

Copy link
Copy Markdown
Contributor Author

@nachocano nachocano Mar 24, 2020

Choose a reason for hiding this comment

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

sorrry, was AFK...
The problem that the informer is not registered: Unable to fetch knative.dev/eventing/pkg/client/informers/externalversions/eventing/v1alpha1.EventTypeInformer from context

updated the codegen stuff... will be back in a few mins

@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Mar 24, 2020 via email

@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Mar 24, 2020

looks like you got a /home/prow/go/src/knative.dev/eventing is out of date. Please run hack/update-codegen.sh

@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Mar 24, 2020

/retest

@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Mar 24, 2020

hmmm
TestFanoutMessageHandler_ServeHTTP/all_subs_succeed
must be some race going on?

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Mar 24, 2020

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/source/crd/controller.go Do not exist 52.9%
pkg/reconciler/source/crd/crd.go Do not exist 33.8%
pkg/reconciler/source/duck/controller.go Do not exist 86.7%
pkg/reconciler/source/duck/duck.go Do not exist 70.2%
pkg/reconciler/source/duck/resources/eventtype.go Do not exist 100.0%

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Mar 24, 2020 •
edited
Loading

@nachocano: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-eventing-go-coverage cf56517 link /test pull-knative-eventing-go-coverage

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Mar 24, 2020

@n3wscott mind if we get this one in as in and we keep on debugging that one to further increase the coverage?

@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Mar 24, 2020

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2020
@knative-prow-robot knative-prow-robot merged commit f0e0a81 into knative:master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lionelvillard lionelvillard Awaiting requested review from lionelvillard

@liu-cong liu-cong Awaiting requested review from liu-cong

2 more reviewers

@mattmoor mattmoor mattmoor left review comments

@n3wscott n3wscott n3wscott left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@n3wscott n3wscott

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nachocano @knative-prow-robot @n3wscott @knative-metrics-robot @mattmoor @googlebot