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

S13nServiceWorker: Add tests for network fallback for navigations with request bodies #9193

Merged
merged 1 commit into from Jan 30, 2018

Conversation

Projects
None yet
7 participants
@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Jan 25, 2018

Originally I planned to clone the request body for main resource requests,
similar to subresource requests, but it looks unnecessary for now because
main resource request bodies don't have data pipe getter elements. They
are only created by the renderer when converting from a Blob, which doesn't
happen for navigations.

So this patch:

  • Adds a DCHECK to the main resource request handling code that there are
    no data pipe elements.
  • Adds a WPT test for network fallback with a request body (using just strings
    since we can't test uploading a file with WPT as far as a I can tell, it needs
    user interaction or a special test harness flag).
  • Adds a http/tests/local/ test case for file upload with network fallback.
    This test file was already passing, so the failing expectation is also removed.

R=kinuko, shimazu

Bug: 778878
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I7a5f7a72ef9ac2ca3ffbe54549739ca6bcc8d071
Reviewed-on: https://chromium-review.googlesource.com/885684
Commit-Queue: Matt Falkenhagen falken@chromium.org
Reviewed-by: Kinuko Yasuda kinuko@chromium.org
Reviewed-by: Makoto Shimazu shimazu@chromium.org
Cr-Commit-Position: refs/heads/master@{#532121}

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Jan 25, 2018

Build ERRORED

Started: 2018-01-29 16:39:45
Finished: 2018-01-29 16:48:35

Failing Jobs

  • chrome:dev
  • firefox:nightly

Unstable Results

Browser: "Firefox Nightly"

View in: WPT PR Status | TravisCI

Test Subtest Results Messages
/service-workers/service-worker/fetch-event.https.html   OK: 2
TIMEOUT: 8
  FetchEvent#body is a blob PASS: 2
NOTRUN: 8
  FetchEvent#body is a blob and is passed to network fallback PASS: 2
NOTRUN: 8
  FetchEvent#body is a string PASS: 2
NOTRUN: 8
  FetchEvent#body is a string and is passed to network fallback PASS: 2
NOTRUN: 8
  Multiple calls of respondWith must throw InvalidStateErrors PASS: 10
  Service Worker does not respond to fetch event PASS: 10
  Service Worker event.respondWith must set the used flag PASS: 10
  Service Worker falls back to network in fetch event with POST form TIMEOUT: 6
FAIL: 2
NOTRUN: 2
assert_equals: expected "testName1=testValue1&testName2=testValue2" but got ""
Test timed out
  Service Worker fetches other file in fetch event PASS: 10
  Service Worker headers in the request of a fetch event PASS: 10
  Service Worker responds to fetch event with an existing client id FAIL: 10
assert_unreached: unexpected rejection: assert_equals: Service Worker should respond to fetch with a client id expected "Client ID Not Found" but got "Client ID Found: null" Reached unreachable code
  Service Worker responds to fetch event with blob body PASS: 10
  Service Worker responds to fetch event with null response body PASS: 10
  Service Worker responds to fetch event with POST form TIMEOUT: 2
PASS: 6
FAIL: 2
assert_equals: expected "POST:application/x-www-form-urlencoded:testName1=testValue1&testName2=testValue2" but got "{\"error\": {\"message\": \"\", \"code\": 404}}"
Test timed out
  Service Worker responds to fetch event with string PASS: 10
  Service Worker responds to fetch event with the correct cache types PASS: 10
  Service Worker responds to fetch event with the correct integrity_metadata PASS: 10
  Service Worker responds to fetch event with the correct keepalive value FAIL: 2
NOTRUN: 8
assert_equals: expected "false" but got ""
  Service Worker responds to fetch event with the referrer URL PASS: 10
  Service Worker should expose FetchEvent URL fragments. PASS: 10
  Service Worker should intercept EventSource PASS: 10

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-885684 branch from 412e44b to accf14e Jan 25, 2018

@tobie

This comment has been minimized.

Copy link
Contributor

tobie commented Jan 25, 2018

Summoning @wpt-pr-bot.

@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-885684 branch from accf14e to 986308f Jan 26, 2018

S13nServiceWorker: Add tests for network fallback for navigations wit…
…h request bodies

Originally I planned to clone the request body for main resource requests,
similar to subresource requests, but it looks unnecessary for now because
main resource request bodies don't have data pipe getter elements. They
are only created by the renderer when converting from a Blob, which doesn't
happen for navigations.

So this patch:
- Adds a DCHECK to the main resource request handling code that there are
no data pipe elements.
- Adds a WPT test for network fallback with a request body (using just strings
since we can't test uploading a file with WPT as far as a I can tell, it needs
user interaction or a special test harness flag).
- Adds a http/tests/local/ test case for file upload with network fallback.
This test file was already passing, so the failing expectation is also removed.

R=kinuko, shimazu

Bug: 778878
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I7a5f7a72ef9ac2ca3ffbe54549739ca6bcc8d071
Reviewed-on: https://chromium-review.googlesource.com/885684
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532121}
@mattto

This comment has been minimized.

Copy link
Member

mattto commented Jan 30, 2018

I see the error. The mix of async_test and promise_test in fetch-event.https.html is bad news: add_result_callback() in a promise_test gets called for all the prior async tests . I plan to land the fix directly in Chromium. Let me know if I should revert and then reland with the fix instead.

@mattto

This comment has been minimized.

Copy link
Member

mattto commented Jan 30, 2018

This test was fixed in Chromium's codebase at https://chromium-review.googlesource.com/c/chromium/src/+/892553, I'm not sure why WPT bot didn't seem to pick it up yet.

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jan 30, 2018

@foolip Could you force merge this PR? The flakiness was resolved in a follow-up CL https://crrev.com/c/892553

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Jan 30, 2018

@matto The follow-up CL hasn't been exported because it depends on this CL. Once this CL is merged (manually), the follow-up CL will be exported automatically.

BTW, could you add your chromium.org email address to your GitHub account? That way, GitHub will be able to link these exported commits to your GitHub account. Thanks!

@foolip foolip merged commit a99e6b6 into master Jan 30, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@foolip foolip deleted the chromium-export-cl-885684 branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.