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 hpack-test-case based tests #976

Merged
merged 1 commit into from Jul 18, 2014
Merged

Add hpack-test-case based tests #976

merged 1 commit into from Jul 18, 2014

Conversation

ghost
Copy link

@ghost ghost commented Jun 30, 2014

For isssue #931 this creates a submodule referencing http2jp/hpack-test-case, and runs the decoder across each of the directories for interop testing. Many of these fail, so they're in an @ignored bad test.

Another test added does a round trip of headers through our encoder and decoder -- this test is not quite as effective as having known good output of our own in http2jp/hpack-test-case so that's next.

import com.squareup.okhttp.internal.spdy.hpackjson.Story;
import okio.Buffer;

import java.util.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No star imports.

Copy link
Author

Choose a reason for hiding this comment

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

done

@swankjesse
Copy link
Collaborator

LGTM. Please address issues above, and squash your commits!

TODO
----

* Make hpack-test-case update itself from git, and notify if there are new
Copy link
Collaborator

Choose a reason for hiding this comment

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

ultranit: one space before, one space after bullet?

 * Make hpack-test-case....

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

done?

Copy link
Author

Choose a reason for hiding this comment

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

Nope you've gotta add new interop tests by hand
On Jul 1, 2014 8:42 AM, "Jesse Wilson" notifications@github.com wrote:

In okhttp-hpacktests/README.md:

@@ -0,0 +1,16 @@
+OkHttp HPACK tests
+==================
+
+These tests use the [hpack-test-case][1] project to validate OkHttp's HPACK
+implementation.
+
+TODO
+----
+
+* Make hpack-test-case update itself from git, and notify if there are new

done?


Reply to this email directly or view it on GitHub
https://github.com/square/okhttp/pull/976/files#r14411738.

@swankjesse
Copy link
Collaborator

Do you need to git push -f ?

@ghost
Copy link
Author

ghost commented Jul 2, 2014

OK all set ramping up on tools.

@codefromthecrypt
Copy link

Some comments might not have been addressed, ex. the copyright. Please scan back through and make sure the commit that is visible includes them. Also, I would take a little time to make the README clear at least on the process to initialize the project and what to do when the hpack tests upstream project changes. Since we are linking to something else, we want to ensure the maintenance aspect of this isn't subject to bus factor.

otherwise LGTM and I will look at the failing tests for relevance, etc once merged.

@ghost
Copy link
Author

ghost commented Jul 6, 2014

k should be all set

@swankjesse
Copy link
Collaborator

Just got back from vacation and finally circling back to this. This looks awesome. I'm eager to put it to use.

swankjesse added a commit that referenced this pull request Jul 18, 2014
@swankjesse swankjesse merged commit e6686f6 into square:master Jul 18, 2014
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.

5 participants