Skip to content

Comments

Fix EnsureCleanup to properly skip clean up when requested#16372

Merged
knative-prow[bot] merged 2 commits intoknative:mainfrom
dprotaso:fix-e2e-test-cleanup
Feb 9, 2026
Merged

Fix EnsureCleanup to properly skip clean up when requested#16372
knative-prow[bot] merged 2 commits intoknative:mainfrom
dprotaso:fix-e2e-test-cleanup

Conversation

@dprotaso
Copy link
Member

@dprotaso dprotaso commented Feb 6, 2026

EnsureCleanup is called after creating some resource to
register clean up handlers. No invocation of this function
uses defer so typically the test has not failed. When passing
in the SkipCleanupOnFail=true flag we were then never skipping
said cleanup because of it.

EnsureCleanup is called after creating some resource to
register clean up handlers. No invocation of this function
uses defer so typically the test has not failed. When passing
in the SkipCleanupOnFail=true flag we were then never skipping
said cleanup because of it.
@knative-prow
Copy link

knative-prow bot commented Feb 6, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 6, 2026
@knative-prow knative-prow bot requested review from linkvt and skonto February 6, 2026 16:31
@dprotaso
Copy link
Member Author

dprotaso commented Feb 6, 2026

Looks like the change was introduced in #15217
/assign @mgencur
/assign @linkvt

@codecov
Copy link

codecov bot commented Feb 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.21%. Comparing base (2cd582b) to head (20b72e7).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16372      +/-   ##
==========================================
+ Coverage   80.18%   80.21%   +0.02%     
==========================================
  Files         216      216              
  Lines       13440    13440              
==========================================
+ Hits        10777    10781       +4     
+ Misses       2298     2295       -3     
+ Partials      365      364       -1     

☔ View full report in Codecov by Sentry.
📢 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.

@dprotaso
Copy link
Member Author

dprotaso commented Feb 6, 2026

@mgencur your comment in your PR said

Fix a bug related to ServingFlags.SkipCleanupOnFail. The cleanup should really be skipped only if the test fails. This was missed during reviews on the #12482

I think this is accurate - let me tweak what I did

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 6, 2026
@dprotaso
Copy link
Member Author

dprotaso commented Feb 6, 2026

/retest

@linkvt
Copy link
Contributor

linkvt commented Feb 7, 2026

/lgtm
/hold for Martins review

@knative-prow knative-prow bot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 7, 2026
@mgencur
Copy link
Contributor

mgencur commented Feb 9, 2026

/lgtm

@mgencur
Copy link
Contributor

mgencur commented Feb 9, 2026

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2026
@knative-prow knative-prow bot merged commit 5e94a30 into knative:main Feb 9, 2026
155 of 156 checks passed
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants