Managed Handler: add some basic protocol tests for status line handling #23333

geoffkizer · 2017-08-17T09:54:58Z

stephentoub · 2017-08-17T14:13:59Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+        }
+    }
+
+    public sealed class ManagedHandler_HttpProtocolTests : HttpProtocolTests, IDisposable


Seems like we should have non-ManagedHandler versions of these to ensure the exceptions we get back match the existing behavior.

stephentoub · 2017-08-17T14:14:19Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+        public override Stream GetStream(Stream s)
+        {
+            return new DribbleStream(s);
+        }


Nit: could be

public override Stream GetStream(Stream s) => new DribbleStream(s);

stephentoub · 2017-08-17T14:15:40Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+        [InlineData(555, "we just don't like you")]
+        [InlineData(600, "still valid")]
+        [InlineData(999, "this\ttoo\t")]
+        public async Task ValidStatusLine(int statusCode, string reason)


Is there any benefit https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L1292 provides over this? Seems like we need one or the other but not both.

davidsh · 2017-08-17T14:16:00Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+        [InlineData(600, "still valid")]
+        [InlineData(999, "this\ttoo\t")]
+        public async Task ValidStatusLine(int statusCode, string reason)
+        {


Please follow the current naming convention for networking tests which use a three part name separated by underscores.

stephentoub · 2017-08-17T14:16:19Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+        [InlineData("HTTP/A.1 200 OK")]
+        [InlineData("HTTP/X.Y.Z 200 OK")]
+        [InlineData("HTTP\\1.1 200 OK")]
+        [InlineData("HTTP 1.1 200 OK")]


Similarly for https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L1317... can that be rolled into this and that one deleted?

davidsh · 2017-08-17T14:16:57Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+
+namespace System.Net.Http.Functional.Tests
+{
+    public abstract class HttpProtocolTests


Please follow the naming convention for class name and filename regarding test classes which use a singular "Test" as the suffix.

stephentoub · 2017-08-17T14:17:19Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+                await s.WriteAsync(responseBytes, 0, responseBytes.Length);
+                accept.Close();
+                listen.Close();
+            }, TaskContinuationOptions.ExecuteSynchronously);


I think this is now the 3rd or 4th loopback server we have in these tests :) Can we augment one of the existing ones however is needed rather than adding another?

I agree. We already have a LoopbackServer that we use extensively in Http tests. And you can have whatever status line you want returned to enable "fuzzer" type tests.

Yeah, I'll use the existing LoopbackServer.

Priya91 · 2017-08-18T17:57:58Z

src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs

+
+        [Theory]
+        [InlineData(200, "OK")]
+        [InlineData(200, "Sure why not?")]


how about obs-text chars in range %x80-FF, also percent encoded chars, like "Is this valid? %FF"

Priya91

Test cases LGTM!

benaadams · 2017-08-19T14:20:58Z

@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build

karelz · 2017-09-06T16:41:59Z

@geoffkizer what is status of this PR? 2.5 weeks no update ...

karelz · 2017-10-02T16:45:50Z

@geoffkizer ping?

stephentoub · 2017-10-17T16:30:44Z

I'll finish this.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Managed Handler: add some basic protocol tests for status line handling Commit migrated from dotnet/corefx@08802ff

dnfclas added the cla-already-signed label Aug 17, 2017

stephentoub reviewed Aug 17, 2017

View reviewed changes

davidsh added the area-System.Net.Http label Aug 17, 2017

stephentoub reviewed Aug 17, 2017

View reviewed changes

davidsh reviewed Aug 17, 2017

View reviewed changes

stephentoub reviewed Aug 17, 2017

View reviewed changes

davidsh reviewed Aug 17, 2017

View reviewed changes

stephentoub reviewed Aug 17, 2017

View reviewed changes

karelz assigned geoffkizer Aug 17, 2017

Priya91 reviewed Aug 18, 2017

View reviewed changes

Priya91 approved these changes Aug 18, 2017

View reviewed changes

Geoff Kizer added 2 commits October 17, 2017 11:49

add some basic protocol tests for status line handling

79709c4

add more cases and make tests more robust

a3cab9a

stephentoub assigned stephentoub and unassigned geoffkizer Oct 17, 2017

stephentoub force-pushed the httpprotocoltests branch from 315b83d to 3b6870c Compare October 18, 2017 03:33

Address PR feedback

3b9525b

stephentoub force-pushed the httpprotocoltests branch from 3b6870c to 3b9525b Compare October 18, 2017 04:19

stephentoub merged commit 08802ff into dotnet:master Oct 18, 2017

karelz added this to the 2.1.0 milestone Oct 28, 2017

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Apr 30, 2019

Add more span-based Vector<T> ctors and CopyTo methods (dotnet#23333)

4fc1390

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

GrabYourPitchforks added a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request May 1, 2019

Add more span-based Vector<T> ctors and CopyTo methods (dotnet#23333)

326f273

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

stephentoub pushed a commit that referenced this pull request May 1, 2019

Add more span-based Vector<T> ctors and CopyTo methods (#23333)

6b88ea8

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#23333 from geoffkizer/httpprotocoltests

33b3d5d

Managed Handler: add some basic protocol tests for status line handling Commit migrated from dotnet/corefx@08802ff

Managed Handler: add some basic protocol tests for status line handling #23333

Managed Handler: add some basic protocol tests for status line handling #23333

Uh oh!

Conversation

geoffkizer commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya91 Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya91 left a comment

Choose a reason for hiding this comment

Uh oh!

benaadams commented Aug 19, 2017

Uh oh!

karelz commented Sep 6, 2017

Uh oh!

karelz commented Oct 2, 2017

Uh oh!

stephentoub commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Priya91 Aug 18, 2017 •

edited

Loading