change wait calls for check calls in tests

[leetcope]

Fixes #1178

Proposed Changes

• Bring in changes from knative/pkg to use CheckEndpointState

[codecov]

Codecov Report

Merging #11680 (ff7cd6e) into main (0be381e) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #11680      +/-   ##
==========================================
- Coverage   87.85%   87.78%   -0.08%     
==========================================
  Files         196      196              
  Lines        9373     9430      +57     
==========================================
+ Hits         8235     8278      +43     
- Misses        884      893       +9     
- Partials      254      259       +5     

Impacted Files	Coverage Δ
pkg/reconciler/route/reconcile_resources.go	76.66% <0.00%> (-4.94%)	⬇️
pkg/reconciler/route/controller.go	100.00% <0.00%> (ø)
pkg/apis/serving/v1/route_lifecycle.go	100.00% <0.00%> (ø)
pkg/reconciler/route/route.go	79.89% <0.00%> (+0.19%)	⬆️
pkg/reconciler/configuration/configuration.go	88.46% <0.00%> (+1.53%)	⬆️
pkg/reconciler/route/resources/service.go	93.02% <0.00%> (+1.77%)	⬆️

---

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 0be381e...ff7cd6e. Read the comment docs.

[markusthoemmes]

Have you considered #11404 (and by extension #5573) in this work?

[leetcope]

/retest

[leetcope]

@markusthoemmes thanks for the remainder, I had forgotten about the route state check

[leetcope]

/assign @dprotaso

[markusthoemmes]

@markusthoemmes thanks for the remainder, I had forgotten about the route state check

I'm not quite parsing that response 🤔. My question was whether this impacts the discussion in the issue I shared (I think it does?).

[leetcope]

@markusthoemmes well I didn't change every place where a WaitFor function is used, at first I did but some actually need the WaitFor function, so I don't think it made the tests any narrower

[dprotaso]

I still see a few more waits after we check/wait for our resources to be Ready

ie. here's a raw dump - not all apply

$ rg WaitForEndpointState

test/conformance/runtime/util.go
58:	resp, err := pkgTest.WaitForEndpointState(

test/conformance/runtime/readiness_probe_test.go
114:				if _, err = pkgtest.WaitForEndpointState(

test/e2e/scale.go
186:				_, err = pkgTest.WaitForEndpointState(
195:					t.Error("WaitForEndpointState(expected text) =", err)
196:					return fmt.Errorf("WaitForEndpointState(expected text) failed: %w", err)

test/conformance/api/v1/util.go
131:	_, err := pkgTest.WaitForEndpointState(

test/conformance/api/v1/route_test.go
43:	_, err := pkgtest.WaitForEndpointState(

test/e2e/autoscale.go
237:	if _, err := pkgTest.WaitForEndpointState(

test/e2e/logging_test.go
108:		// A request was sent to / in WaitForEndpointState.

test/e2e/autoscale_hpa_test.go
94:	if _, err := pkgTest.WaitForEndpointState(

test/e2e/tagheader/tag_header_based_routing_test.go
118:			if _, err := pkgTest.WaitForEndpointState(

test/performance/benchmarks/scale-from-zero/continuous/main.go
198:		_, err := pkgTest.WaitForEndpointStateWithTimeout(

test/ha/ha.go
66:	if _, err := pkgTest.WaitForEndpointState(

test/upgrade/upgrade.go
53:	if _, err := pkgTest.WaitForEndpointState(

$ rg WaitForRouteState

test/e2e/subroutes_test.go
160:	if err = v1test.WaitForRouteState(clients.ServingClient, resources.Route.Name, func(r *v1.Route) (bool, error) {
185:	if err := v1test.WaitForRouteState(clients.ServingClient, resources.Route.Name, func(r *v1.Route) (bool, error) {
271:	if err = v1test.WaitForRouteState(clients.ServingClient, resources.Route.Name, func(r *v1.Route) (bool, error) {
303:	if err = v1test.WaitForRouteState(clients.ServingClient, resources.Route.Name, func(r *v1.Route) (b bool, e error) {
347:	if err = v1test.WaitForRouteState(clients.ServingClient, resources.Route.Name, v1test.IsRouteReady, "Route is ready"); err != nil {

test/e2e/destroypod_test.go
72:	if err := v1test.WaitForRouteState(clients.ServingClient, names.Route, v1test.IsRouteReady, "RouteIsReady"); err != nil {

test/conformance/api/v1/generatename_test.go
191:	if err := v1test.WaitForRouteState(clients.ServingClient, names.Route, v1test.IsRouteReady, "RouteIsReady"); err != nil {

test/conformance/api/v1/route_test.go
36:	if err := v1test.WaitForRouteState(clients.ServingClient, names.Route, v1test.IsRouteReady, "RouteIsReady"); err != nil {

test/e2e/route_service_test.go
161:			if err = v1test.WaitForRouteState(clients.ServingClient, serviceresourcenames.Route(svc), hasPrivateRoute, "RouteIsClusterLocal"); err != nil {

test/e2e/autotls/auto_tls_test.go
74:	if err = v1test.WaitForRouteState(clients.ServingClient, names.Route, routeTLSDisabled, "RouteTLSDisabled"); err != nil {
78:	if err = v1test.WaitForRouteState(clients.ServingClient, names.Route, routeURLHTTP, "RouteURLIsHTTP"); err != nil {
136:		if err = v1test.WaitForRouteState(clients.ServingClient, names.Route, routeTrafficHTTPS, "RouteTrafficIsHTTPS"); err != nil {

test/v1/route.go
69:// WaitForRouteState polls the status of the Route called name from client every
73:func WaitForRouteState(client *test.ServingClients, name string, inState func(r *v1.Route) (bool, error), desc string) error {
74:	span := logging.GetEmitableSpan(context.Background(), fmt.Sprintf("WaitForRouteState/%s/%s", name, desc))
97:// This is the non-polling variety of WaitForRouteState

[leetcope]

/retest

[dprotaso]

Looks like there's a new test file added with the rebase test/e2e/emptydir_test.go that uses WaitFor when it can be a Check

I still see Waits in the following files - any reason why those can't be switch over?

test/conformance/api/v1beta1/domain_mapping_test.go
test/conformance/api/v1/util.go
test/conformance/runtime/readiness_probe_test.go
test/e2e/autoscale.go

[dprotaso]

Can you update the comments in line 108 in this file?

[dprotaso]

Can you update the error strings in this file - lines 196 and 197 to Match this new method call

[dprotaso]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, shinigambit

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:

• test/OWNERS [dprotaso]

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

[nak3]

The Check() func used by CheckEndpointState drops DefaultErrorRetryChecker so this change made e2e narrower 😢
We probably need to make DefaultErrorRetryChecker available by some option?

[dprotaso]

Interesting we should probably add that to the Check call in knative.dev/pkg - cc @shinigambit