update test context.WithCancel with testing.T.Context#3583
update test context.WithCancel with testing.T.Context#3583olamilekan000 wants to merge 1 commit intokcp-dev:mainfrom
Conversation
|
Hi @olamilekan000. Thanks for your PR. I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
af48695 to
e8fc8b3
Compare
e8fc8b3 to
8c7f9c9
Compare
|
|
||
| ctx, cancel := context.WithCancelCause(context.Background()) | ||
| defer cancel(errors.New("test has ended")) | ||
| ctx := t.Context() |
There was a problem hiding this comment.
Please replace uses of ctx with t.Context() - cases where we used ctx, cancel := ... was only due to needing access to cancel.
There was a problem hiding this comment.
There's only one occurance of context.WithCancelCause at the moment and the reason it wasn't changed is because the cancel function is being assigned as a field in authenticatorState.
c.authConfigAuthenticators[shard][mapKey] = authenticatorState{
cancel: cancel,
authenticator: authn,
}should it be removed?
There was a problem hiding this comment.
The cancel func needs to remain in the authState, or else the front-proxy will not be able to stop OIDC authenticators at runtime. But this should not affect this test function at all.
I would say you changed this test function here exactly as expected.
There was a problem hiding this comment.
Actually, the part of the code I shared doesn't need to change. There are no longer usage of context.WithCancelCause for test in this branch.
8c7f9c9 to
c2a4c7a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/ok-to-test |
|
/retest |
|
lint failure looks leggit |
|
/retest |
Signed-off-by: olalekan odukoya <[email protected]>
c2a4c7a to
0386116
Compare
|
@olamilekan000: The following tests failed, say
DetailsInstructions 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. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions 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. |
|
Superseded by #3795 |
Summary
What Type of PR Is This?
/kind cleanup
Related Issue(s)
Fixes #
Release Notes