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 Pipelines-related http features in TestServer #8660

Closed
analogrelay opened this issue Mar 19, 2019 · 10 comments
Closed

Add Pipelines-related http features in TestServer #8660

analogrelay opened this issue Mar 19, 2019 · 10 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. ✔️ Resolution: Duplicate Resolved as a duplicate of another issue help wanted Up for grabs. We would accept a PR to help resolve this issue Status: Resolved
Milestone

Comments

@analogrelay
Copy link
Contributor

analogrelay commented Mar 19, 2019

Epic #8829
Split from #7815

We should support the Pipe Goodness on TestServer.

We'd remove the stream-based implementation and use pipes. Stream-based implementation would then be a wrapper around the pipes.

@JamesNK
Copy link
Member

JamesNK commented Mar 26, 2019

Something to test/implement with this issue: ensure that Response.StartAsync correctly sets Response.HasStarted to true in TestServer. Right now I don't think calling flush on the response stream, e.g. Response.Body.FlushAsync sets it correctly.

@analogrelay
Copy link
Contributor Author

Bumping out of preview4. I don't think this is a critical thing for preview 4, but feel free to debate :).

@JamesNK
Copy link
Member

JamesNK commented Mar 26, 2019

I would like it. Does that make it critical? 🙃

Seriously though, I was looking into grpc/grpc-dotnet#144 yesterday and TestServer never sets Response.HasStarted when the response content is flushed, and that makes our functional tests a little less trustworthy. For example, it is not an error to send headers after writing content. I'm considering moving some of the functional tests to use a spun up Kestrel instance because I don't trust how accurate TestServer is.

But I understand there is higher priority work.

@analogrelay analogrelay added PRI: 3 - Optional enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Apr 30, 2019
@analogrelay
Copy link
Contributor Author

analogrelay commented May 8, 2019

We just don't have the capacity for this in 3.0. I'll put this in preview7 for now, and if someone feels so inspired, we can review a PR :)

@analogrelay
Copy link
Contributor Author

@JamesNK does your PR satisfy this need? Are we ready to close this?

@JamesNK
Copy link
Member

JamesNK commented May 16, 2019

My PR satisfies my need to have StartAsync work correctly.

TestServer still needs to be refactored to use pipes, but from my POV you can bump it to post 3.0 if you don't have time.

@analogrelay
Copy link
Contributor Author

Ok, we'll keep this around. There may be capacity later when the unknown unknowns become known knowns.

@analogrelay analogrelay modified the milestones: 3.0.0-preview7, Backlog Jun 4, 2019
@analogrelay
Copy link
Contributor Author

We don't expect to have the capacity in 3.0. Lots on the queue. This is a good candidate for 3.1 though.

@analogrelay analogrelay added help wanted Up for grabs. We would accept a PR to help resolve this issue good first issue Good for newcomers. labels Jun 4, 2019
@alefranz
Copy link
Contributor

I had a look at this and I believe that most of work has been done as part of #11598.

@analogrelay
Copy link
Contributor Author

Looks like you're correct and #11598 has a better listing of what is done and not yet done. I'm going to close this as a duplicate of it.

@analogrelay analogrelay added the ✔️ Resolution: Duplicate Resolved as a duplicate of another issue label Jan 21, 2020
@ghost ghost added the Status: Resolved label Jan 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Feb 20, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers. ✔️ Resolution: Duplicate Resolved as a duplicate of another issue help wanted Up for grabs. We would accept a PR to help resolve this issue Status: Resolved
Projects
None yet
Development

No branches or pull requests

5 participants