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

[WIP] Add reconciler test for EventType references #7114

Closed
wants to merge 9 commits into from

Conversation

Leo6Leo
Copy link
Member

@Leo6Leo Leo6Leo commented Jul 26, 2023

Fixes #7031

Proposed Changes

Add reconciler test for EventType references

  • A test showing an EventType reconciling properly with a Broker reference
  • A test showing an EventType reconciling properly with a Channel reference

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note


Docs

@knative-prow
Copy link

knative-prow bot commented Jul 26, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2023
@knative-prow knative-prow bot requested review from evankanderson and mgencur July 26, 2023 20:41
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Jul 26, 2023
@knative-prow
Copy link

knative-prow bot commented Jul 26, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Leo6Leo
Once this PR has been reviewed and has the lgtm label, please assign upodroid for approval. For more information see the Kubernetes Code Review Process.

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

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

@Leo6Leo
Copy link
Member Author

Leo6Leo commented Jul 26, 2023

Write down some initial attempt on this ticket. Am I on the right track? If yes, my question would be: how can we specify the eventType for a event here?

@creydr @pierDipi

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (25400fb) 77.73% compared to head (560a7f2) 77.93%.
Report is 145 commits behind head on main.

❗ Current head 560a7f2 differs from pull request most recent head 9a81be4. Consider uploading reports for the commit 9a81be4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7114      +/-   ##
==========================================
+ Coverage   77.73%   77.93%   +0.20%     
==========================================
  Files         246      248       +2     
  Lines       13284    13315      +31     
==========================================
+ Hits        10326    10377      +51     
+ Misses       2435     2405      -30     
- Partials      523      533      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Leo6Leo Leo6Leo changed the title [WIP] Add some initial attempt code [WIP] Add reconciler test for EventType references Jul 26, 2023
test/rekt/eventtype_test.go Outdated Show resolved Hide resolved
Leo6Leo added 2 commits July 27, 2023 16:09
…stion: how to retrieve all the eventType in the event registry by using go code
"testing"
"time"

"eventing/test/rekt/features/eventtype"
Copy link
Member Author

Choose a reason for hiding this comment

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

Question: I added the new module eventtype in rekt. How should I import the module here?

Please lmk if this is not the right way to approach this ticket.

/cc @matzew @pierDipi @creydr

Copy link
Member

Choose a reason for hiding this comment

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

Seems you're using an incorrect import path. The knative modules are all under knative.dev/<repo>/... (no matter if it is a knative-sandbox project or not). So in this case it would be "knative.dev/eventing/test/rekt/features/eventtype"

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the approach: In the test files (under test/rekt//_test.go), we usually setup only the environment and call the feature test (as you have done). In the feature test (test/rekt/features/) we have the according test implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that before. If I use knative.dev/eventing/test/rekt/features/eventtype, it will throw this error

test/rekt/eventtype_test.go:47:32: undefined: eventtype.eventTypeWithBrokerAsReference

Does importing the packages that starting with knative.dev/*** will actually download the latest knative package from the source? Because this eventTypeWithBrokerAsReference is a new function that I added in this PR, it hasn't been merged into the main branch. @creydr

Copy link
Member

Choose a reason for hiding this comment

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

It's probably, because you have to make eventTypeWithBrokerAsReference public

@knative-prow knative-prow bot requested review from creydr, matzew and pierDipi July 28, 2023 21:25
@Leo6Leo
Copy link
Member Author

Leo6Leo commented Aug 15, 2023

/hold
Unhold until #7166 #7180 are merged

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2023
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2023
@knative-prow-robot
Copy link
Contributor

PR needs rebase.

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.

Copy link

github-actions bot commented Jan 4, 2024

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2024
@Leo6Leo
Copy link
Member Author

Leo6Leo commented Jan 4, 2024

/remove-lifecycle stale

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2024
@Leo6Leo
Copy link
Member Author

Leo6Leo commented Jan 5, 2024

Close as this PR wrong

@Leo6Leo Leo6Leo closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test-and-release Test infrastructure, tests or release do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add reconciler test for EventType references
3 participants