test: improve default correlation unit tests #501
Conversation
✅ Deploy Preview for arcus-observability ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| DefaultCorrelationInfoAccessor.Instance.SetCorrelationInfo( | ||
| new CorrelationInfo(operationId, transactionId)); | ||
| await Task.Run(() => DefaultCorrelationInfoAccessor.Instance.SetCorrelationInfo( | ||
| new CorrelationInfo(operationId, transactionId))); | ||
| } |
There was a problem hiding this comment.
Well, we avoid the warning here but it is actually a bad practice, as it is by no means necessary to startup another thread just for this. See also this: https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-using.html
Why is the SetCorrelationInfoAsync method even declared as async ? I don't think that is even necessary ?
Moreover, I think this complete testcase can be synchronous ?
There was a problem hiding this comment.
This test case was made because we had a problem with setting our correlation in an asynchronously setting. It really is a unit test to secure this fix.
Also note that this is testing deprecated functionality.
There was a problem hiding this comment.
Ah.
In that case, it would have been good if the name of the test described that we're specifically testing this in an async context.
There was a problem hiding this comment.
Added ...InAsyncContext to the test's name.
| DefaultCorrelationInfoAccessor.Instance.SetCorrelationInfo( | ||
| new CorrelationInfo(operationId, transactionId)); | ||
| await Task.Run(() => DefaultCorrelationInfoAccessor.Instance.SetCorrelationInfo( | ||
| new CorrelationInfo(operationId, transactionId))); | ||
| } |
There was a problem hiding this comment.
Ah.
In that case, it would have been good if the name of the test described that we're specifically testing this in an async context.
Improve asynchronous default correlation unit tests.
Closes #344