-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[sdk#809] Refresh should not happen after failed request #1017
[sdk#809] Refresh should not happen after failed request #1017
Conversation
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
}) | ||
require.Error(t, err) | ||
|
||
goleak.VerifyNone(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
goleak.VerifyNone(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To validate that there is no pending refresh action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow we already have the check on cleaning at line 273
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bolodya1997 Could you please explain this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not the same, because [1] check we are making after the test context cancel.
We can remove [1], but with it test looks more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use clockmock instead of an additional goleak call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, fixed.
@@ -267,3 +268,24 @@ func TestRefreshClient_Sandbox(t *testing.T) { | |||
return refreshSrv.getState() == testRefreshStateDoneRequest | |||
}, sandboxTotalTimeout, sandboxStepDuration) | |||
} | |||
|
|||
func TestRefreshClient_NoRefreshOnFailure(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me this test is incorrect due to there are no expire time in request.
- Please add expiration time and use
clockmock
to check that there are no new requests after getting an error on the request. - Please add test for Close use-case. As I can see you've added a major return statement at pkg/networkservice/common/refresh/client.go:133. So this scenario should be covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, added.
1490e22
to
6b35ff5
Compare
6b35ff5
to
8734238
Compare
Description
Disable refresh if request fails.
Issue link
#809
How Has This Been Tested?
Types of changes