Skip to content

test(delivery): cover backoff maximum through triggers - #9330

Merged
knative-prow[bot] merged 2 commits into
knative:mainfrom
kahirokunn:test/delivery-backoff-max-trigger
Sep 21, 2026
Merged

test(delivery): cover backoff maximum through triggers#9330
knative-prow[bot] merged 2 commits into
knative:mainfrom
kahirokunn:test/delivery-backoff-max-trigger

Conversation

@kahirokunn

@kahirokunn kahirokunn commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Related to #9278.

TL;DR: Extend backoffMax coverage from Subscription-based delivery to Trigger-based delivery so the shared delivery configuration stays aligned across both paths.

Proposed Changes

  • 🧪 Add experimental Broker-to-Trigger E2E coverage using the same delivery-count and retry-timing assertions as the Channel-to-Subscription path.
  • 🧪 Verify that direct Trigger delivery settings and Broker fallback settings propagate backoffMax to the internal Subscription.
  • 🧹 Extract shared backoff assertions and add Trigger manifest support for rendering backoffMax in rekt tests.

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact (Not applicable: this change only adds test coverage and test helpers.)
  • Spec PR for any new API feature (Not applicable: this change does not modify the API.)
  • Conformance test for any change to the spec (Not applicable: this change does not modify the spec.)

Release Note

NONE

Docs

Not applicable. This change only adds test coverage and test helpers.

@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 Aug 28, 2026
@kahirokunn
kahirokunn force-pushed the test/delivery-backoff-max-trigger branch from 52c2011 to dc4f47a Compare August 28, 2026 14:22
@codecov

codecov Bot commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.24%. Comparing base (68ee8ee) to head (3873c09).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9330      +/-   ##
==========================================
+ Coverage   51.17%   51.24%   +0.06%     
==========================================
  Files         411      411              
  Lines       22179    22179              
==========================================
+ Hits        11351    11366      +15     
+ Misses       9950     9932      -18     
- Partials      878      881       +3

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kahirokunn
kahirokunn force-pushed the test/delivery-backoff-max-trigger branch from dc4f47a to b8419ca Compare September 1, 2026 08:41
dsimansk
dsimansk reviewed Sep 18, 2026
View reviewed changes
Comment thread test/experimental/features/backoff_max/assertions.go Outdated
dsimansk
dsimansk reviewed Sep 18, 2026
View reviewed changes
Comment thread test/experimental/features/backoff_max/assertions.go Outdated

for i, wait := range expected {
actual := deliveries[i+1].Time.Sub(deliveries[i].Time)
if actual < wait-500*time.Millisecond || actual > wait+3*time.Second {

@dsimansk dsimansk Sep 18, 2026

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.

The upper bound +3s seems to benevolent? I'm not a fan of this construction of range assertion.

@kahirokunn kahirokunn Sep 20, 2026

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.

Thank you 🙏
After researching, I found that testing/synctest is an effective approach for this issue, so I used it to address the problem.

dsimansk
dsimansk reviewed Sep 18, 2026
View reviewed changes

@dsimansk dsimansk left a comment

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.

@kahirokunn looks really good. I've commented on 2 nit, but I'd not call them blocking.

@kahirokunn
kahirokunn force-pushed the test/delivery-backoff-max-trigger branch from b8419ca to ec73290 Compare September 20, 2026 12:07
@knative-prow knative-prow Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2026
@kahirokunn
kahirokunn force-pushed the test/delivery-backoff-max-trigger branch 2 times, most recently from a73508b to b582886 Compare September 20, 2026 12:14
@kahirokunn
test(subscription): cover explicit backoff maximum propagation
bbd2cc4 
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
@kahirokunn
test(delivery): cover backoff maximum through triggers
3873c09 
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
@kahirokunn
kahirokunn force-pushed the test/delivery-backoff-max-trigger branch from b582886 to 3873c09 Compare September 20, 2026 12:23
@kahirokunn

kahirokunn commented Sep 20, 2026

Copy link
Copy Markdown
Member Author

@dsimansk Thank you for the review! I have addressed your feedback. I would appreciate it if you could take another look. Thank you!

@dsimansk

dsimansk commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

/approve
/lgtm

@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2026
@knative-prow

knative-prow Bot commented Sep 21, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, kahirokunn

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 knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2026
@knative-prow
knative-prow Bot merged commit 7c892cf into knative:main Sep 21, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsimansk dsimansk dsimansk left review comments
@evankanderson evankanderson Awaiting requested review from evankanderson
@Leo6Leo Leo6Leo Awaiting requested review from Leo6Leo

Assignees

@dsimansk dsimansk

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release 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.

2 participants

@kahirokunn @dsimansk