"Wait" is overly used in e2e/conformance

[mattmoor]

The conformance (and possibly e2e) tests have a number of checks that poll for success. This masks problems where we say we're Ready but aren't actually.

We should change the e2e test to use Wait methods a bit more sparingly and then concretely check the state of the system.

If this is too flaky, we can consider adding retries, but the idea of "Wait" feels wrong to me.

/assign @mattmoor

I'll see how flaky this is after we get it re-enabled with Wait in place.

[mattmoor]

Sent out a PoC (linked above).

[mattmoor]

It is notable that the small change I made in the PoC above caught a bug I almost introduced in #1322.
The Revision's status would have converged to what we expected after a resync, but because we were "Check"ing vs. "Wait"ing it blocked my PR. I'd like to see us tighten up more of this checking to hopefully squeeze out other issues we might be missing.

/kind API
/kind spec
/assign @jonjohnsonjr

[knative-housekeeping-robot]

Issues go stale after 90 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle stale.
Stale issues rot after an additional 30 days of inactivity and eventually close.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle stale

[knative-housekeeping-robot]

Stale issues rot after 30 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle rotten.
Rotten issues close after an additional 30 days of inactivity.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle rotten

[knative-housekeeping-robot]

Rotten issues close after 30 days of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh by adding the comment /remove-lifecycle rotten.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/close

[knative-prow-robot]

@knative-housekeeping-robot: Closing this issue.

In response to this:

Rotten issues close after 30 days of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh by adding the comment /remove-lifecycle rotten.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/close

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.

[vagababov]

/reopen
Still need to fix this
/kind good-first-issue

[knative-prow-robot]

@vagababov: Reopened this issue.

In response to this:

/reopen
Still need to fix this
/kind good-first-issue

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.

[knative-housekeeping-robot]

Rotten issues close after 30 days of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh by adding the comment /remove-lifecycle rotten.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/close

[mattmoor]

/lifecycle frozen

[Cynocracy]

Is it enough to just scan for occurrences of Wait() and see if there is extra state checking that can be done?
Or just the TODOs you mentioned, maybe?

[mattmoor]

This history of this is that "Ready" didn't used to mean "Ready", so we'd wait for Ready and then we'd wait for it to really be ready.

Things like this should be "Check" not "WaitFor" because we already waited for Ready, and that should mean something:

serving/test/conformance/api/v1/route_test.go

Line 42 in 1642dd1

_, err := pkgTest.WaitForEndpointState(

I'd probably start with the TODOs, but in general WaitForEndpointState is probably another good thing to grep for.

[Cynocracy]

Sounds good. thanks for the info! If I have spare cycles I think this sounds interesting to pick up,.

[markusthoemmes]

FWIW this is related: #5573. Sadly we still haven't fixed that downstream.

[mattmoor]

Here's a great example fwiw:

serving/test/conformance/api/v1/single_threaded_test.go

Lines 54 to 55 in 20616d5

	// Ready does not actually mean Ready for a Route just yet.
	// See https://github.com/knative/serving/issues/1582

[evankanderson]

This probably either needs a target list or some other explicit criteria on when it can be closed.

/assign @dprotaso

/triage needs-user-input

[dprotaso]

/unassign @dprotaso
/assign @shinigambit will take a look at this

[knative-prow-robot]

@dprotaso: GitHub didn't allow me to assign the following users: at, this, will, take, a, look.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/unassign @dprotaso
/assign @shinigambit will take a look at this

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.

[evankanderson]

/remove-triage needs-user-input
/triage accepted