Add a fetch test for upload streaming #4362

yutakahirano · 2016-12-19T11:10:55Z

Adding a test for uploading a request made from a ReadableStream.

wpt-pr-bot · 2016-12-19T11:11:01Z

Notifying @jdm and @youennf. (Learn how reviewing works.)

yutakahirano · 2016-12-19T11:11:45Z

annevk · 2016-12-19T12:45:54Z

This looks okay to me. Though ideally we also have some tests around redirect handling and different kinds of methods. For a new feature that will need tests written anyway as part of the implementation this might be good enough though.

@foolip @domenic thoughts?

foolip · 2016-12-19T13:36:46Z

Yeah, testing non-trivial things blindly is very hard, and a good reason to not write all tests up front. Keeping track somewhere of what testing coverage is missing would be nice of course, but only if someone is then using that to guide testing priorities.

domenic · 2016-12-19T15:41:46Z

Wait, we're allowing strings to be enqueued? I didn't get that from whatwg/fetch#425 .

annevk · 2016-12-19T15:45:58Z

That is a good point and needs to be fixed in the test.

Where do we currently place restrictions on what can be on a ReadableStream object used in Fetch? It seems like we don't...

yutakahirano · 2016-12-19T16:13:27Z

Wait, we're allowing strings to be enqueued? I didn't get that from whatwg/fetch#425 .

Sorry that was my mistake. Fixed.

Where do we currently place restrictions on what can be on a ReadableStream object used in Fetch? It seems like we don't...

At least, Request.text(), etc. expect that as they are using https://fetch.spec.whatwg.org/#concept-read-all-bytes-from-readablestream.

annevk · 2016-12-19T16:18:31Z

We should also make the network layer expect that though. And whatever ends up reading a response.

yutakahirano · 2016-12-19T16:22:10Z

At https://fetch.spec.whatwg.org/#read-a-request?

annevk · 2016-12-19T17:22:44Z

Yeah I suppose. And we probably need something similar for responses then and make sure all specifications use it… 😟

annevk · 2016-12-19T18:49:24Z

I'm not sure what you're asking for.

guest271314 · 2016-12-19T18:52:50Z

@annevk Tried to run the current test in browser at http://w3c-test.org/tools/runner/index.html. Checked all fetch tests, browser crashed at a worker test. Just want to run the current test. How to do this at *nix, chromium 52?

annevk · 2016-12-19T18:54:33Z

What current test? This is a pull request for a test change. It hasn't landed...

guest271314 · 2016-12-19T18:58:37Z

@annevk This test https://github.com/w3c/web-platform-tests/pull/4362/files/3a994b7107795de307eb7039676e130fbf11d9e9..54138da06de3bca241195f1459ae863b4ade77e8. Not well-versed in the terminology and mechanics of git, pull requests. Is it not currently possible to try this now? Is a "pull request" code written that is tested before being published at large?

annevk · 2016-12-19T19:00:49Z

You can locally check out the code and run it. I believe for some PRs there's also a URL you can visit, but not for this PR it seems (might depend on where the branch is situated).

guest271314 · 2016-12-19T19:05:21Z

@annevk Is the above referenced test in that repository? Do not need to run all of the tests in the repo, just the newest one, or only the fetch tests. Is it necessary to download the entire repo? And install the python server? Was there changes made to the way fetch body functions in order to pass the test? Perhaps beyond the scope of this topic, though am trying to learn more about this part of programming. This is the only experience have so far with this part of programming https://github.com/guest271314/jquery-3.0.0-promises-tests-in-browser , http://stackoverflow.com/questions/36438228/how-to-test-if-jquery-3-0-beta-is-promises-a-compatible-in-browser/

annevk · 2016-12-19T19:16:13Z

The test of this PR is in https://github.com/yutakahirano/web-platform-tests/tree/upload-streaming, a branch in a fork of this repository. If you just want to know whether any browser passes the test, I can tell you that none do, since as I told you elsewhere, this is not implemented yet.

guest271314 · 2016-12-19T19:38:06Z

@annevk Am attempting to gain more knowledge about the entire process. Your feedback has been helpful.

Basic test: web-platform-tests/wpt#4362. More tests are expected to be written as part of the implementation effort. Further work: #441. Fixes #88.

annevk · 2017-01-05T09:45:27Z

@yutakahirano perhaps you can also add a test that expects strings and numbers and such to fail.

yutakahirano · 2017-01-05T12:32:42Z

@yutakahirano perhaps you can also add a test that expects strings and numbers and such to fail.

Done.

annevk · 2017-01-05T12:44:02Z

Thanks, @domenic could you give it another review?

domenic

Some minor test nits. Additionally I have something I am confused about; forgive me if we have discussed it before.

These tests seem to imply that fetch() will read the whole request body stream before fulfilling or rejecting the fetch promise. But, doesn't that hurt our ability to do full-duplex communication with streams? I thought the idea was that you could pass a request body stream, and get back a response body stream, and use them both at the same time. If we wait for the request body stream to be fully read before vending the response body stream, this is not possible, right?

domenic · 2017-01-05T18:17:42Z

fetch/api/basic/request-upload.js

+      body = body();
+    if (body)
+      requestInit["body"] = body;
+    return fetch(url, requestInit).then(() => {


This should use return promise_rejects(new TypeError(), fetch(url, requestInit))

domenic · 2017-01-05T18:18:31Z

fetch/api/basic/request-upload.js

+testUploadFailure("Fetch with POST with ReadableStream containing number", url, "POST", new ReadableStream({start: controller => {
+    controller.enqueue(99);
+    controller.close();
+  }}));


We should also test ArrayBuffer and Blob are not allowed

annevk · 2017-01-05T18:35:14Z

I agree with your assessment of how things should be with regards to full-duplex communication. These tests are simply not testing that scenario, or am I missing something?

domenic · 2017-01-05T19:03:23Z

Well, these tests assert bad data causes a rejected promise. How is that possible unless you read to the end before delivering the response?

With full duplex you should be able to get a fulfilled promise (with a response you can read from) even if the request contains bad data. Maybe there's a race there and this test is assuming the race resolves in favor of terminating due to bad data before the response is vended?

annevk · 2017-01-05T19:08:24Z

I would expect that the default server setup is such that it doesn't emit a response before getting the full request body. It would be good to have tests for early transmitted responses though.

domenic · 2017-01-05T19:19:01Z

I see. So is the intent of the spec that there's a race then? If the fetch gets terminated before the promise settles, then the promise is rejected (as in this case); if the fetch gets terminated after the response comes back, the promise is still fulfilled?

annevk · 2017-01-05T19:31:14Z

Yeah, the fetch gets terminated so you might not get the whole response body. We should really figure out a way to write full duplex tests since I suspect it is all kinds of broken.

domenic · 2017-01-05T19:31:45Z

OK, if that's intended then the tests are good, just the nits in my review remaining.

Basic tests: web-platform-tests/wpt#4362. More tests are expected to be written as part of the implementation effort. Fixes #88, fixes yutakahirano/fetch-with-streams#10, fixes yutakahirano/fetch-with-streams#57, and fixes yutakahirano/fetch-with-streams#66.

done

3a994b7

wpt-pr-bot added the fetch label Dec 19, 2016

fix

54138da

yutakahirano mentioned this pull request Dec 20, 2016

Define "to read a request" more specifically whatwg/fetch#441

Closed

fix

326a612

annevk requested a review from domenic January 5, 2017 12:44

domenic requested changes Jan 5, 2017

View reviewed changes

fix

f9b0cf1

domenic approved these changes Jan 6, 2017

View reviewed changes

annevk merged commit 1dd5361 into web-platform-tests:master Jan 7, 2017

annevk mentioned this pull request Jan 7, 2017

Developer-controlled streams for request/response whatwg/fetch#88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a fetch test for upload streaming #4362

Add a fetch test for upload streaming #4362

yutakahirano commented Dec 19, 2016

wpt-pr-bot commented Dec 19, 2016

yutakahirano commented Dec 19, 2016

annevk commented Dec 19, 2016

foolip commented Dec 19, 2016 •

edited

Loading

domenic commented Dec 19, 2016

annevk commented Dec 19, 2016

yutakahirano commented Dec 19, 2016

annevk commented Dec 19, 2016

yutakahirano commented Dec 19, 2016

annevk commented Dec 19, 2016

annevk commented Dec 19, 2016

guest271314 commented Dec 19, 2016 •

edited

Loading

annevk commented Dec 19, 2016

guest271314 commented Dec 19, 2016 •

edited

Loading

annevk commented Dec 19, 2016 •

edited

Loading

guest271314 commented Dec 19, 2016 •

edited

Loading

annevk commented Dec 19, 2016

guest271314 commented Dec 19, 2016 •

edited

Loading

annevk commented Jan 5, 2017

yutakahirano commented Jan 5, 2017

annevk commented Jan 5, 2017

domenic left a comment

domenic Jan 5, 2017

yutakahirano Jan 6, 2017

domenic Jan 5, 2017

yutakahirano Jan 6, 2017

annevk commented Jan 5, 2017

domenic commented Jan 5, 2017

annevk commented Jan 5, 2017

domenic commented Jan 5, 2017

annevk commented Jan 5, 2017

domenic commented Jan 5, 2017

Add a fetch test for upload streaming #4362

Add a fetch test for upload streaming #4362

Conversation

yutakahirano commented Dec 19, 2016

wpt-pr-bot commented Dec 19, 2016

yutakahirano commented Dec 19, 2016

annevk commented Dec 19, 2016

foolip commented Dec 19, 2016 • edited Loading

domenic commented Dec 19, 2016

annevk commented Dec 19, 2016

yutakahirano commented Dec 19, 2016

annevk commented Dec 19, 2016

yutakahirano commented Dec 19, 2016

annevk commented Dec 19, 2016

annevk commented Dec 19, 2016

guest271314 commented Dec 19, 2016 • edited Loading

annevk commented Dec 19, 2016

guest271314 commented Dec 19, 2016 • edited Loading

annevk commented Dec 19, 2016 • edited Loading

guest271314 commented Dec 19, 2016 • edited Loading

annevk commented Dec 19, 2016

guest271314 commented Dec 19, 2016 • edited Loading

annevk commented Jan 5, 2017

yutakahirano commented Jan 5, 2017

annevk commented Jan 5, 2017

domenic left a comment

Choose a reason for hiding this comment

domenic Jan 5, 2017

Choose a reason for hiding this comment

yutakahirano Jan 6, 2017

Choose a reason for hiding this comment

domenic Jan 5, 2017

Choose a reason for hiding this comment

yutakahirano Jan 6, 2017

Choose a reason for hiding this comment

annevk commented Jan 5, 2017

domenic commented Jan 5, 2017

annevk commented Jan 5, 2017

domenic commented Jan 5, 2017

annevk commented Jan 5, 2017

domenic commented Jan 5, 2017

foolip commented Dec 19, 2016 •

edited

Loading

guest271314 commented Dec 19, 2016 •

edited

Loading

guest271314 commented Dec 19, 2016 •

edited

Loading

annevk commented Dec 19, 2016 •

edited

Loading

guest271314 commented Dec 19, 2016 •

edited

Loading

guest271314 commented Dec 19, 2016 •

edited

Loading