Add CheckEndpointState method to SpoofingClient #2166
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2166 +/- ##
==========================================
+ Coverage 66.05% 66.51% +0.46%
==========================================
Files 224 224
Lines 9426 9472 +46
==========================================
+ Hits 6226 6300 +74
+ Misses 2920 2881 -39
- Partials 280 291 +11
Continue to review full report at Codecov.
|
test/request.go
Outdated
| @@ -84,6 +84,8 @@ func EventuallyMatchesBody(expected string) spoof.ResponseChecker { | |||
| } | |||
| } | |||
|
|
|||
| type endpointStateCallable = func(ctx context.Context, u *url.URL, inState spoof.ResponseChecker, desc string, r ...spoof.RequestOption) (*spoof.Response, error) | |||
There was a problem hiding this comment.
do we think there are ever likely to be more than two possible things "implementing" this? If not I'd be very tempted to just pass down a wait bool parameter here
|
/retest |
Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Sameer Vohra <vsameer@vmware.com>
…checks Signed-off-by: Fabian Lopez <lfabian@vmware.com>
Signed-off-by: Fabian Lopez <lfabian@vmware.com>
31c7c87 to
eeafd0d
Compare
Signed-off-by: Fabian Lopez <lfabian@vmware.com>
test/request.go
Outdated
| return waitForEndpointStateWithTimeout(ctx, kubeClient, logf, url, inState, desc, resolvable, timeout, true, opts) | ||
| } | ||
|
|
||
| func waitForEndpointStateWithTimeout( |
There was a problem hiding this comment.
It's a bit confusing to call this waitForEndpointStateWithTimeout but there's a wait argument that makes this check.
I think we should just change this to only create the spoof client and then in the respective Wait Check methods call client.Wait/Check
test/spoof/spoof.go
Outdated
| if ok { | ||
| sc.logZipkinTrace(resp) | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
The be consistent with Poll we should log if resp != nil - thus you'll want to do the err check after this
There was a problem hiding this comment.
It's very similar to the Poll method but I think resp is guaranteed to be non nil at this point so I just moved the log line out of the if ok check. Let me know if that's fine
test/spoof/spoof_test.go
Outdated
| sc, err := New( | ||
| context.TODO(), | ||
| fKlient, | ||
| t.Logf, | ||
| "some.svc.knative.dev", | ||
|
|
||
| false, | ||
| "host", | ||
| 1, | ||
| 1, | ||
| ) |
There was a problem hiding this comment.
I think it'll be simpler to setup the tests if you just construct a SpoofingClient struct directly vs. calling the New constructor
test/spoof/spoof_test.go
Outdated
| wantCalls: 1, | ||
| }} | ||
| for _, tt := range tests { | ||
| _, fKlient := fake.With(context.TODO(), ingress) |
There was a problem hiding this comment.
I would just create the k8s fake client directly vs pulling in the injection library
Signed-off-by: Fabian Lopez <lfabian@vmware.com>
|
/lgtm |
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
CheckEndpointStatejust for checking an endpoint's response/kind api-change
Part of knative/serving#1178
Release Note