Skip to content
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

add StreamConformanceTest for HTTP CONNECT stream #50699

Closed

Conversation

geoffkizer
Copy link
Contributor

@stephentoub @dotnet/ncl

@ghost
Copy link

ghost commented Apr 4, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

@stephentoub @dotnet/ncl

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Assert.True(pair.Stream2.CanRead);
return pair;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this testing a different stream than Http1UpgradeResponseStreamConformanceTests is testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same stream, but a different way of constructing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a "we know the implementation" perspective, this doesn't seem like it's actually testing anything further:

                else if (ReferenceEquals(normalizedMethod, HttpMethod.Connect) && response.StatusCode == HttpStatusCode.OK)
                {
                    responseStream = new RawConnectionStream(this);
                    _connectionClose = true; 
                }
                else if (response.StatusCode == HttpStatusCode.SwitchingProtocols)
                {
                    responseStream = new RawConnectionStream(this);
                }

but if you really think it's worth adding additional stream tests for, ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right here. I'll just close this.

@geoffkizer geoffkizer closed this Apr 5, 2021
@geoffkizer geoffkizer deleted the httpconnectconformancetests branch April 5, 2021 19:07
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants