Conversation
| // continual auth renewal if set | ||
| if c.Renewal > 0 { | ||
| ticker := clock.Ticker(time.Duration(c.Renewal)) | ||
| go c.authRenewal(context.Background(), ticker, log) |
There was a problem hiding this comment.
The background context is never going to get cancelled, I assume this is by design here. In the unit test you use a context with cancel to be able to cleanly stop the authRenewal goroutine.
Using the background context here confused me for a few minutes. Maybe it would be good to add a comment explaining that it's only cancelled in tests, so background is appropriate in Start()?
There was a problem hiding this comment.
I added a comment explaining its use, you are right this context was just intended for tests.
|
@jh125486 I think you wrote this earlier this year. How does this fix for intermittent unit test failures look to you? |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks good, but this is honestly the first time I've seen/used circleci, so I'm by no ones an expert on this. |
The tests in
cookie_test.goare still failing randomly (link to circleci failed Mac build on master), I thought I had resolved this issue in this pr #9608. I believe the problem is due to a race condition, where the go-routine running the ticker loop to authenticate again isn't guaranteed to be called before checking the count.This pr adds a
sync.WaitGroupand acontext.Contextwith cancel, to guarantee that the ticker loop has finished running before checking the count. Also moved the authentication renewal loop to a separate function so that the tests can pass ticker and then callticker.Close(). Refactored the tests away from using the assert function as well to reduce repeated code.