Reproduction of bug #5442

[cardil]

Reproduces #5442

Proposed Changes

• Adds test that reproduce Trigger can pass duck.WaitForResourceReady on GKE without subscriber being deployed yet #5442

[codecov]

Codecov Report

Merging #5443 (907edb1) into main (38bfba7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5443   +/-   ##
=======================================
  Coverage   82.68%   82.68%           
=======================================
  Files         200      200           
  Lines        6261     6261           
=======================================
  Hits         5177     5177           
  Misses        750      750           
  Partials      334      334           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38bfba7...907edb1. Read the comment docs.

[mgencur]

@cardil thanks for the reproducer but is this supposed to be merged? I don't think so...

[cardil]

Why do not merge?

Failing test is skipped, so when it gets fixed one could just remove the skip.

[mgencur]

I think it would be better to attach the reproducer to the bug and merge it together with the fix. Merging this now doesn't bring any benefits.

[cardil]

I think it would be better to attach the reproducer to the bug and merge it together with the fix. Merging this now doesn't bring any benefits.

The bug was fixed in knative/pkg#2149, so the way you mentioned isn't possible.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cardil
To complete the pull request process, please assign chaodaig after the PR has been reviewed.
You can assign the PR to them by writing /assign @chaodaig in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

• test/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[lionelvillard]

@cardil are you still working on this?

[cardil]

It should be now fixed (a while back, in fact - 0.24+), judging by #5442 (comment).

I'll double-check that.

[cardil]

Hmm, we could add those negative tests to test suite - they will ensure that trigger will not be ready without valid subscriber.

The problem with that is that, when testing for negative things, we need to wait for timeout on the tested method. That has 2 problems:

• It adds quite a long time to the suite. In this case, that's about 2min being added. We could battle that time to make it less, for example by allowing to pass context.Context (see test/lib/creation.go funcs should accept context.Context #5322) to methods like duck.WaitForResourceReady, but it will always adds some time.
• It's hard to choose a proper, safe, timeout. Let's say I choose 5min as timeout, and test passes as trigger haven't became ready in that time, but maybe it will became ready in 5m1s. I can't be sure about it.

I'm seeing two solutions for this problem:

• create a seperate test suite for negative testing, and run all tests in parallel, if possible, setting a large timeout - like 1h, and run such suite on nightly tests
• don't bother to test for negative cases at all

[github-actions]

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.