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.

Sign up for GitHub

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

Jump to bottom

Update MockWebServer to use Headers. #1270

Merged
merged 1 commit into from
Jan 5, 2015
Merged

Update MockWebServer to use Headers. #1270

merged 1 commit into from
Jan 5, 2015

Conversation

JakeWharton
Copy link
Collaborator

No description provided.

@JakeWharton
Copy link
Collaborator Author

Public API changes to MockResponse and RecordedRequest to expose Headers rather than hide as implementation detail will come next.

@swankjesse
Copy link
Collaborator

LGTM

Rats, the tests are hanging!

@JakeWharton
Copy link
Collaborator Author

Oh, snap. These changes don't look like they could have broken anything either...

@JakeWharton
Copy link
Collaborator Author

I can get com.squareup.okhttp.mockwebserver.MockWebServerTest.expect100ContinueWithNoBody to hang via CLI but it does not hang when run in IDEA.

@JakeWharton JakeWharton force-pushed the jw/mock-web-headers branch 3 times, most recently from 6537183 to 0bf76a2 Compare January 3, 2015 02:51
@JakeWharton
Copy link
Collaborator Author

I can reproduce this locally but the thread dump isn't clear to me what the problem is.

@JakeWharton JakeWharton mentioned this pull request Jan 3, 2015
Merged
@JakeWharton JakeWharton force-pushed the jw/mock-web-headers branch 2 times, most recently from 7a6cca6 to 963875f Compare January 4, 2015 09:07
@JakeWharton
Copy link
Collaborator Author

I'm at a loss for why this still hangs. The thread dumps clearly indicate a SPDY client is waiting to read but that MWS isn't doing anything.

JakeWharton
JakeWharton reviewed Jan 4, 2015
View reviewed changes
mockwebserver/src/main/java/com/squareup/okhttp/mockwebserver/MockResponse.java
return headerList;
}

Headers getNewHeaders() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will eventually replace getHeaders() but that requires a good bit of refactoring which will be in a follow-up.

codefromthecrypt
codefromthecrypt reviewed Jan 4, 2015
View reviewed changes
okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/HttpOverHttp20Draft16Test.java
MockResponse response = new MockResponse().setBody("ABCDE").setStatus("HTTP/1.1 200 Sweet")
.withPush(new PushPromise("GET", "/foo/bar", Arrays.asList("foo: bar"),
new MockResponse().setBody("bar").setStatus("HTTP/1.1 200 Sweet")));
PushPromise pushPromise = new PushPromise("GET", "/foo/bar", Headers.of("foo", "bar"),
Copy link

Choose a reason for hiding this comment

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

looks nicer

@codefromthecrypt
Copy link

confirmed.. after updating to latest JRE (needed as alpn is pinned to recent ssl classes), master builds fine, but this branch hangs on CacheAdapterTest. Looking into it.

@codefromthecrypt
Copy link

Maybe this doesn't fail in idea because the bootclasspath doesn't have the alpn library set?

@JakeWharton JakeWharton force-pushed the jw/mock-web-headers branch 2 times, most recently from 7941820 to 2c630a7 Compare January 5, 2015 01:19
@JakeWharton
Copy link
Collaborator Author

When I add the ALPN jar to the test config in the IDE it still passes.

@JakeWharton
Copy link
Collaborator Author

I found it!!!

@JakeWharton
Copy link
Collaborator Author

Filed at #1294

JakeWharton
JakeWharton reviewed Jan 5, 2015
View reviewed changes
mockwebserver/src/main/java/com/squareup/okhttp/mockwebserver/MockWebServer.java
@@ -817,7 +806,7 @@ private RecordedRequest readRequest(SpdyStream stream) throws IOException {
} else if (name.equals(Header.VERSION)) {
version = value;
} else {
httpHeaders.add(name.utf8() + ": " + value);
httpHeaders.add(name.utf8(), value);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the bug. name can start with a colon causing the line-based parsing to blow up.

Copy link

Choose a reason for hiding this comment

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

ahah!

@JakeWharton JakeWharton force-pushed the jw/mock-web-headers branch 2 times, most recently from 6c5344e to 96d39ab Compare January 5, 2015 07:57
@JakeWharton JakeWharton force-pushed the jw/mock-web-headers branch 2 times, most recently from 7d4c88c to f5ae323 Compare January 5, 2015 08:12
@JakeWharton
Copy link
Collaborator Author

Had to make two tiny changes:

  • Expose a means of replacing all headers. This allows tests to use Headers.addLenient to test invalid cached values. MockResponse no longer allows creating invalid headers.
  • Change truncateViolently to not assume the headers List<String> was mutable.

JakeWharton added a commit that referenced this pull request Jan 5, 2015
@JakeWharton
Merge pull request #1270 from square/jw/mock-web-headers
6763803 
Update MockWebServer to use Headers.
@JakeWharton JakeWharton merged commit 6763803 into master Jan 5, 2015
@JakeWharton JakeWharton deleted the jw/mock-web-headers branch January 5, 2015 08:27
@JakeWharton
Copy link
Collaborator Author

(What an ordeal this was)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JakeWharton @swankjesse @codefromthecrypt