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

h2spec fix 5.1.1. Stream Identifiers #83

Merged
merged 3 commits into from Nov 28, 2016

Conversation

georgeu2000
Copy link
Collaborator

  × Sends even-numbered stream identifier
    - The endpoint MUST respond with a connection error of type PROTOCOL_ERROR.

73 tests, 33 passed, 0 skipped, 40 failed

      × Sends even-numbered stream identifier
        - The endpoint MUST respond with a connection error of type PROTOCOL_ERROR.
@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage decreased (-0.3%) to 96.613% when pulling 73bb48f on georgeu2000:h2spec-fix-5.1.1 into 7960144 on igrigorik:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage decreased (-0.3%) to 96.613% when pulling 73bb48f on georgeu2000:h2spec-fix-5.1.1 into 7960144 on igrigorik:master.

@@ -226,6 +226,12 @@ def receive(data)
else
case frame[:type]
when :headers
# When even-numbered stream identifier is received,
# the endpoint MUST respond with a connection error of type PROTOCOL_ERROR.
if frame[ :stream ].even?
Copy link
Owner

Choose a reason for hiding this comment

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

Err, wouldn't this break client apps? As in, connection.rb is used by both client and server classes, and this check effectively forces client-only code path for header processing.

@georgeu2000
Copy link
Collaborator Author

Sorry, I forgot to run the http-2 specs.

I made an update but wasn't sure what you meant by

this check effectively forces client-only code path for header processing

@coveralls
Copy link

coveralls commented Nov 22, 2016

Coverage Status

Coverage decreased (-0.001%) to 96.898% when pulling f52efc6 on georgeu2000:h2spec-fix-5.1.1 into 7960144 on igrigorik:master.

@georgeu2000
Copy link
Collaborator Author

BTW, it's difficult to track how many and which h2specs are passing and failing. I think we need a formatter to concisely list the failures...

@@ -226,6 +226,10 @@ def receive(data)
else
case frame[:type]
when :headers
# When server receives even-numbered stream identifier,
# the endpoint MUST respond with a connection error of type PROTOCOL_ERROR.
connection_error if frame[:stream].even? && is_a?(Server)
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Minor nitpick (surprised rubocop didn't complain): https://github.com/georgeu2000/http-2/blob/f52efc656619f5f05b4f69e203f2ffd1ae4bd10e/lib/http/2/connection.rb#L638 - for consistency, can we self.is_a?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

@georgeu2000
Copy link
Collaborator Author

georgeu2000 commented Nov 23, 2016

Regarding h2spec, perhaps a better solution is to save the output to a file. Then a diff (commit) will show what is fixed and what is broken.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 96.898% when pulling 623304b on georgeu2000:h2spec-fix-5.1.1 into 7960144 on igrigorik:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 96.898% when pulling 623304b on georgeu2000:h2spec-fix-5.1.1 into 7960144 on igrigorik:master.

@coveralls
Copy link

coveralls commented Nov 23, 2016

Coverage Status

Coverage decreased (-0.001%) to 96.898% when pulling 623304b on georgeu2000:h2spec-fix-5.1.1 into 7960144 on igrigorik:master.

@georgeu2000
Copy link
Collaborator Author

@igrigorik - Haven't heard back so I guess it is OK. Merging...

@georgeu2000 georgeu2000 merged commit 7987ad4 into igrigorik:master Nov 28, 2016
@georgeu2000 georgeu2000 deleted the h2spec-fix-5.1.1 branch November 28, 2016 21:36
@igrigorik
Copy link
Owner

👍

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.

None yet

3 participants