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

Test setting response.body and fetching a message on a server #9321

Merged
merged 1 commit into from Jan 19, 2016

Conversation

@nikkisquared
Copy link
Contributor

nikkisquared commented Jan 14, 2016

I've updated http_fetch to now set response.body, as well as written a test to ensure that fetch can retrieve a message on a server. I've also looked into partially implementing some more of http_fetch while trying to figure out where response.body gets written to.

As always I'd like feedback on my logic, I'm confident there are more steps for handling response.body I need but I find the specification difficult to parse on this.

Review on Reviewable

@@ -422,7 +422,7 @@ pub struct StreamedResponse<R: HttpResponse> {
impl<R: HttpResponse> Read for StreamedResponse<R> {
#[inline]
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
match self.decoder {
match self.decoder {

This comment has been minimized.

@KiChjang

KiChjang Jan 14, 2016

Member

This looks like a mistake.

This comment has been minimized.

@nikkisquared

nikkisquared Jan 14, 2016

Author Contributor

yep, whoops, I caught and fixed that before, but then I must've accidentally undone it when I was trying to fix a different mistake I made. for some reason test-tidy doesn't catch it (assuming it's supposed to be unhappy about spaces not divisible by 4)

@KiChjang
Copy link
Member

KiChjang commented Jan 15, 2016

@jdm
Copy link
Member

jdm commented Jan 15, 2016

Make the requested change, then squash please!

@@ -42,3 +42,25 @@ fn test_fetch_response_is_not_network_error() {
panic!("fetch response shouldn't be a network error");
}
}

// TODO this test requires response body to be set by Fetch

This comment has been minimized.

@jdm

jdm Jan 15, 2016

Member

This comment is fixed now.

let _ = server.close();

match fetch_response.body {
ResponseBody::Receiving(body) | ResponseBody::Done(body) => {

This comment has been minimized.

@jdm

jdm Jan 15, 2016

Member

Let's only match Done here.

@nikkisquared nikkisquared force-pushed the nikkisquared:response_body branch from f349d3c to 68fa9d3 Jan 15, 2016
@KiChjang
Copy link
Member

KiChjang commented Jan 15, 2016

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2016

📌 Commit 68fa9d3 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2016

Testing commit 68fa9d3 with merge 718eba2...

bors-servo added a commit that referenced this pull request Jan 15, 2016
Test setting response.body and fetching a message on a server

I've updated http_fetch to now set response.body, as well as written a test to ensure that fetch can retrieve a message on a server. I've also looked into partially implementing some more of http_fetch while trying to figure out where response.body gets written to.

As always I'd like feedback on my logic, I'm confident there are more steps for handling response.body I need but I find the specification difficult to parse on this.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9321)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2016

💔 Test failed - mac-rel-wpt

@@ -304,7 +306,8 @@ fn http_fetch(request: Rc<Request>,
// Step 3
if !request.skip_service_worker.get() && !request.is_service_worker_global_scope {

// TODO: Substep 1 (handle fetch unimplemented)
// Substep 1

This comment has been minimized.

@KiChjang

KiChjang Jan 15, 2016

Member

Trailing whitespace here.

@bors-servo
Copy link
Contributor

bors-servo commented Jan 18, 2016

The latest upstream changes (presumably #9362) made this pull request unmergeable. Please resolve the merge conflicts.

@nikkisquared nikkisquared force-pushed the nikkisquared:response_body branch from 68fa9d3 to 4165786 Jan 18, 2016
@nikkisquared
Copy link
Contributor Author

nikkisquared commented Jan 18, 2016

I've rebased the branch on upstream/master so it includes #9362 ! And I've made sure to run test-tidy so I don't miss any more whitespace mistakes.

@KiChjang
Copy link
Member

KiChjang commented Jan 18, 2016

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 18, 2016

📌 Commit 4165786 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 18, 2016

Testing commit 4165786 with merge 175b3c2...

bors-servo added a commit that referenced this pull request Jan 18, 2016
Test setting response.body and fetching a message on a server

I've updated http_fetch to now set response.body, as well as written a test to ensure that fetch can retrieve a message on a server. I've also looked into partially implementing some more of http_fetch while trying to figure out where response.body gets written to.

As always I'd like feedback on my logic, I'm confident there are more steps for handling response.body I need but I find the specification difficult to parse on this.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9321)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 18, 2016

💔 Test failed - linux-rel

@KiChjang
Copy link
Member

KiChjang commented Jan 18, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 18, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 19, 2016

@bors-servo bors-servo merged commit 4165786 into servo:master Jan 19, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nikkisquared nikkisquared deleted the nikkisquared:response_body branch Jan 19, 2016
@emilio emilio removed the S-tests-failed label Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.