This repository was archived by the owner on Dec 18, 2018. It is now read-only.
clean up HttpConnectionTests #1208
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add some helpers to reduce duplication * Refactor some tests to clarify what they test * Removed a few redundant test cases
Member
|
wtf is a SyncPoint... |
Contributor
Author
|
Read the code and find out. It's a pair of TCSes that replace a bunch of repeated code in our tests. |
muratg
reviewed
Dec 14, 2017
| @@ -0,0 +1,128 @@ | |||
| using System; | |||
muratg
reviewed
Dec 14, 2017
| @@ -0,0 +1,91 @@ | |||
| using System; | |||
muratg
approved these changes
Dec 14, 2017
muratg
left a comment
muratg
left a comment
There was a problem hiding this comment.
Looks good to me! There could be further clean-up but no need to block this one.
BrennanConroy
approved these changes
Dec 14, 2017
| @@ -0,0 +1,366 @@ | |||
| using System; | |||
| @@ -0,0 +1,104 @@ | |||
| using System; | |||
| @@ -0,0 +1,119 @@ | |||
| using System; | |||
| { | ||
| return Task.FromException<HttpResponseMessage>(new InvalidOperationException($"Http endpoint not implemented: {request.RequestUri}")); | ||
| } | ||
| // Just block until cancelled |
Contributor
Author
|
I'm going to take a look at some of the test flakiness after this. I don't like how hard it is to get the checks to go green. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No changes to product code at all, just to test code.
try { ... } finally { DisposeAsync() }pattern gets noisy fast so I addedWithConnectionAsync()which runs a provided delegate in the body of that try-finally patternTestTransportandTestHttpMessageHandlercode to remove most of the duplication.SyncPointhelper I introduced in Making HttpConnection restartable (C#) #1147 does (having a pair of TCSes to allow test code to wait for code-under-test to reach a certain point and then release it to continue after a condition is met). So I refactored to use that.EventsAreNotRunningOnMainLoopwas depending upon a race. It was testing that whenTryWriteon the channel was called, aTryReadimmediately following would fail because theHttpConnectionreceive loop already read the message. This depends on the way inline-continuations work in Channels and may break with other configurations. The refactored test waits forWaitToReadAsyncto block, which should be more reliableEventQueueTimeoutWithExceptionjust seemed to be checking that exceptions in OnReceive are handled, which other tests already do. So I just removed it.