Skip to content

Conversation

hannobraun
Copy link

No description provided.

MockStream facilitates unit testing of code that uses NetworkStream.
hannobraun added a commit to hannobraun/vndf-2016 that referenced this pull request Oct 22, 2014
I fixed a bug. Pull request pending:
hyperium/hyper#89
@seanmonstar
Copy link
Member

And if the port is 80 or 443, is it omitted? Probably should have a test also.

@hannobraun
Copy link
Author

And if the port is 80 or 443, is it omitted? Probably should have a test also.

No, it isn't. I believe that always adding the port, even if it is the default one, is correct behavior: http://tools.ietf.org/html/rfc7230#section-5.4
However, I've added more test cases to ensure that Host header is correct, if the port in the URL is omitted.

Of course we could omit the port if it is the default one, but since it works as is and I wanted to get on with my other work, simplicity won out in this case :)

Do you want me to omit default ports, or will you merge this as is?

@seanmonstar
Copy link
Member

I'll make some adjustments, starting with this branch. I'll convert the
Host header into a struct with host name and port, and the formatting can
happen in there.
On Oct 22, 2014 9:36 AM, "Hanno Braun" notifications@github.com wrote:

And if the port is 80 or 443, is it omitted? Probably should have a test
also.

No, it isn't. I believe that always adding the port, even if it is the
default one, is correct behavior:
http://tools.ietf.org/html/rfc7230#section-5.4
However, I've added more test cases to ensure that Host header is correct,
if the port in the URL is omitted.

Of course we could omit the port if it is the default one, but since it
works as is and I wanted to get on with my other work, simplicity won out
in this case :)

Do you want me to omit default ports, or will you merge this as is?


Reply to this email directly or view it on GitHub
#89 (comment).

@reem
Copy link
Contributor

reem commented Oct 22, 2014

MockStream belongs in a separate "test" module within hyper for now, and should likely be moved to another crate in the future.

@seanmonstar
Copy link
Member

Merged manually. Thanks @hannobraun!

@hannobraun
Copy link
Author

You're welcome, and thanks for improving on it!

@hannobraun hannobraun deleted the host-header branch October 23, 2014 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants