wpt cookie path tests #11624

g-k · 2016-06-05T18:21:46Z

Adds wpt tests for the cookie path attribute.

Notes:

I included polyfills for fetch and promise to get tests running. Assuming these changes are OK, I'll remove them and add failure expectations so they don't get synced to w3c.
I didn't work out a way to test sending cookies when the request and cookie paths exactly match (e.g. request path /cookies/path/match-exact-page.sub.html and cookie path /cookies/path/match-exact-page.sub.html will not send cookies to /cookies/resources/echo-json.py)
test names could be clearer
everything can be squashed to one commit

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #8756 (github issue number if applicable).
These changes do not require tests because they are tests (and if we test our tests do we need to test our test tests? ...)

This change is

KiChjang · 2016-06-05T21:33:38Z

Fails tidy:

--- a/tests/wpt/metadata/MANIFEST.json
+++ b/tests/wpt/metadata/MANIFEST.json
@@ -36030,18 +36030,18 @@
     "deleted_reftests": {},
     "items": {
       "testharness": {
-        "cookies/path/match-exact-slash.sub.html": [
-          {
-            "path": "cookies/path/match-exact-slash.sub.html",
-            "url": "/cookies/path/match-exact-slash.sub.html"
-          }
-        ],
         "cookies/path/match-exact-page.sub.html": [
           {
             "path": "cookies/path/match-exact-page.sub.html",
             "url": "/cookies/path/match-exact-page.sub.html"
           }
         ],
+        "cookies/path/match-exact-slash.sub.html": [
+          {
+            "path": "cookies/path/match-exact-slash.sub.html",
+            "url": "/cookies/path/match-exact-slash.sub.html"
+          }
+        ],
         "cookies/path/match-prefix-cookie-trailing-slash.sub.html": [
           {
             "path": "cookies/path/match-prefix-cookie-trailing-slash.sub.html",

g-k · 2016-06-05T21:51:12Z

Weird, it's passing for me:

$ git log -1 --format=oneline
713440ecd0a892afb3a12bd2459b54ae476bfca9 fixup! test cookie path
$ ./mach test-tidy
  Progress: 100% (19/19)
Running the WPT lint...
  Progress: 100% (16/16)
tidy reported no errors.

How do I reproduce that?

highfive · 2016-06-05T21:57:38Z

New code was committed to pull request.

KiChjang · 2016-06-06T03:18:27Z

r? @Ms2ger

jdm · 2016-06-06T10:16:21Z

FYI, that's not a tidy check. It's a separate one that runs ./mach update-manifest and verifies the output of git diff.

bors-servo · 2016-06-06T19:44:58Z

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

bors-servo · 2016-06-09T10:00:58Z

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

bors-servo · 2016-06-23T11:56:27Z

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

bors-servo · 2016-06-30T01:48:31Z

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

bors-servo · 2016-07-06T09:53:16Z

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

jdm · 2016-10-04T20:52:30Z

This is looking really great! I'm very interested in that uncaught exception you saw.
-S-awaiting-review +S-needs-code-changes

Reviewed 9 of 21 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

tests/wpt/web-platform-tests/cookies/path/echo-cookie.html, line 13 at r5 (raw file):

  document.cookie = name + '=1; path = ' + path + ';';
}
window.fetchCookieThen = function (name, path) {

The name implies a callback or returning a promise or something.

tests/wpt/web-platform-tests/cookies/path/match.html, line 41 at r5 (raw file):

  var iframe = createIframeThen(function () {
    iframe.contentWindow.fetchCookieThen(testCase.name, testCase.path);
      // TODO: figure out 'ERROR:script::dom::bindings::error: Uncaught exception: failed to extract information' error

Spooky: https://dxr.mozilla.org/servo/source/components/script/dom/bindings/error.rs?q=failed+to+extract&redirect_type=single#221 . If you have a way to reproduce this, that would be really useful.

tests/wpt/web-platform-tests/cookies/path/match.html, line 42 at r5 (raw file):

    iframe.contentWindow.fetchCookieThen(testCase.name, testCase.path);
      // TODO: figure out 'ERROR:script::dom::bindings::error: Uncaught exception: failed to extract information' error
      window.setTimeout(function (testCase, ctx, iframe) {

We'll need to solve the previous problem and remove this setTimeout before we can merge this.

tests/wpt/web-platform-tests/cookies/path/match.html, line 50 at r5 (raw file):

    }
    iframe.contentWindow.expireCookie(testCase.name, testCase.path);
    ctx.done();

nit: spaces instead of tabs.

tests/wpt/web-platform-tests/cookies/resources/set-cookie.py, line 24 at r5 (raw file):

    """
    params = urlparse.parse_qs(request.url_parts.query)
    print >> sys.stderr, params

Let's remove the print statements in this file.

Comments from Reviewable

g-k · 2016-10-04T23:47:21Z

Review status: 1 of 4 files reviewed at latest revision, 5 unresolved discussions.

tests/wpt/web-platform-tests/cookies/path/echo-cookie.html, line 13 at r5 (raw file):

Previously, jdm (Josh Matthews) wrote…

The name implies a callback or returning a promise or something.

Switched it back to returning a promise.

tests/wpt/web-platform-tests/cookies/path/match.html, line 41 at r5 (raw file):

Previously, jdm (Josh Matthews) wrote…

Spooky: https://dxr.mozilla.org/servo/source/components/script/dom/bindings/error.rs?q=failed+to+extract&redirect_type=single#221 . If you have a way to reproduce this, that would be really useful.

I pushed: https://github.com/g-k/servo/tree/failed-to-extract-info-js-error and can reproduce it there but haven't pared it down.

Calling executeTestsSerially directly (without the tests.map call) and removing the iframe made the error go away.

Maybe something to do with the JS VM losing stack info when the iframe promise continues in another window?

tests/wpt/web-platform-tests/cookies/path/match.html, line 42 at r5 (raw file):

Previously, jdm (Josh Matthews) wrote…

We'll need to solve the previous problem and remove this setTimeout before we can merge this.

Fixed on the most recent push.

The fetch API calls or server response isn't setting cookies (tried checking with an alert in e167968) and the assertion failure triggers the promise catch (for some reason I thought the assertion would be self contained and it looks like promise_test does that).

This was the result of trying a few different things like not using an iframe, not passing the callback to the iframe, using post message instead, XHRs instead of the fetch API, etc.

tests/wpt/web-platform-tests/cookies/path/match.html, line 50 at r5 (raw file):

Previously, jdm (Josh Matthews) wrote…

nit: spaces instead of tabs.

Should be fixed or at least

grep '\t' tests/wpt/web-platform-tests/cookies/path/match.html

doesn't return anything.

tests/wpt/web-platform-tests/cookies/resources/set-cookie.py, line 24 at r5 (raw file):

Previously, jdm (Josh Matthews) wrote…

Let's remove the print statements in this file.

Done. https://github.com//pull/11624/commits/6fcbcf8cf1c840f82312dba3a62620a19ed8027e

Comments from Reviewable

jdm · 2016-10-13T13:45:30Z

Almost there! A couple last fixups to maximize the ability to report errors effectively and this should be ready to merge!
-S-awaiting-review +S-needs-code-changes

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

tests/wpt/web-platform-tests/cookies/path/match.html, line 41 at r5 (raw file):

Previously, g-k (Greg Guthe) wrote…

I pushed: https://github.com/g-k/servo/tree/failed-to-extract-info-js-error
and can reproduce it there but haven't pared it down.

Calling executeTestsSerially directly (without the tests.map call) and removing the iframe made the error go away.

Maybe something to do with the JS VM losing stack info when the iframe promise continues in another window?

Thanks! I filed #13748 based on my investigations of this.

tests/wpt/web-platform-tests/cookies/path/match.html, line 25 at r6 (raw file):

};
var testCookiePathFromDOM = function (testCase, test) {
  var iframe = createIframeThen(function () {

createIframeThen(test.step_func(function() {

tests/wpt/web-platform-tests/cookies/path/match.html, line 39 at r6 (raw file):

};
var testCookiePathFromHeader = function (testCase, test) {
  var iframe = createIframeThen(function () {

createIframeThen(test.step_func(function() {

tests/wpt/web-platform-tests/cookies/path/match.html, line 40 at r6 (raw file):

var testCookiePathFromHeader = function (testCase, test) {
  var iframe = createIframeThen(function () {
    iframe.contentWindow.fetchCookieThen(testCase.name, testCase.path).then(function (response) {

.then(test.step_func(function(response) {

Comments from Reviewable


        test cookie path matching

g-k · 2016-10-15T00:48:44Z

Review status: 2 of 4 files reviewed at latest revision, 3 unresolved discussions.

tests/wpt/web-platform-tests/cookies/path/match.html, line 25 at r6 (raw file):

Previously, jdm (Josh Matthews) wrote…

createIframeThen(test.step_func(function() {

Done.

tests/wpt/web-platform-tests/cookies/path/match.html, line 39 at r6 (raw file):

Previously, jdm (Josh Matthews) wrote…

createIframeThen(test.step_func(function() {

Done.

tests/wpt/web-platform-tests/cookies/path/match.html, line 40 at r6 (raw file):

Previously, jdm (Josh Matthews) wrote…

.then(test.step_func(function(response) {

Done.

Comments from Reviewable

jdm · 2016-10-16T14:48:44Z

@bors-servo: r+
Woo!

bors-servo · 2016-10-16T14:48:46Z

📌 Commit ec68a99 has been approved by jdm

bors-servo · 2016-10-16T14:48:48Z

⌛ Testing commit ec68a99 with merge ecbdeb3...


        Auto merge of #11624 - g-k:wpt-cookie-path-tests, r=jdm

wpt cookie path tests Adds wpt tests for the cookie path attribute. Notes: * I included polyfills for fetch and promise to get tests running. Assuming these changes are OK, I'll remove them and add failure expectations so they don't get synced to w3c. * I didn't work out a way to test sending cookies when the request and cookie paths exactly match (e.g. request path `/cookies/path/match-exact-page.sub.html` and cookie path `/cookies/path/match-exact-page.sub.html` will not send cookies to `/cookies/resources/echo-json.py`) * test names could be clearer * everything can be squashed to one commit --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #8756 (github issue number if applicable). - [x] These changes do not require tests because they are tests (and if we test our tests do we need to test our test tests? ...)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11624)

bors-servo · 2016-10-16T15:34:10Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned KiChjang Jun 5, 2016

highfive added the S-awaiting-review label Jun 5, 2016

KiChjang added the S-fails-tidy label Jun 5, 2016

highfive assigned Ms2ger and unassigned KiChjang Jun 6, 2016

KiChjang removed the S-fails-tidy label Jun 6, 2016

highfive added the S-needs-rebase label Jun 6, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from c367912 to 96dd595 Jun 6, 2016

jdm removed the S-needs-rebase label Jun 8, 2016

highfive added the S-needs-rebase label Jun 9, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from 96dd595 to beac1d6 Jun 10, 2016

highfive added the S-needs-rebase label Jun 23, 2016

jdm assigned jdm and unassigned Ms2ger Jun 23, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from beac1d6 to 6dfde22 Jun 25, 2016

highfive added the S-needs-rebase label Jun 30, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from 6dfde22 to c733bd6 Jul 3, 2016

highfive added the S-needs-rebase label Jul 6, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 14, 2016

jdm removed the S-awaiting-review label Oct 4, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from 8817ce4 to b9e37dc Oct 4, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 4, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from b9e37dc to 4ed7305 Oct 4, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 4, 2016

g-k force-pushed the g-k:wpt-cookie-path-tests branch from 4ed7305 to 93b13d0 Oct 4, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 4, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 13, 2016

test cookie path matching

Loading status checks…

ec68a99

g-k force-pushed the g-k:wpt-cookie-path-tests branch from 93b13d0 to ec68a99 Oct 15, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 15, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 16, 2016

bors-servo merged commit ec68a99 into servo:master Oct 16, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 16, 2016

jdm mentioned this pull request Jan 2, 2018

Fix the problems in the cookie implementation that hide test issues #19674

Open

servo / servo

wpt cookie path tests #11624

wpt cookie path tests #11624

g-k commented Jun 5, 2016 •

edited by larsbergstrom

KiChjang commented Jun 5, 2016

g-k commented Jun 5, 2016

highfive commented Jun 5, 2016

KiChjang commented Jun 6, 2016

jdm commented Jun 6, 2016

bors-servo commented Jun 6, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 23, 2016

bors-servo commented Jun 30, 2016

bors-servo commented Jul 6, 2016

jdm commented Oct 4, 2016

g-k commented Oct 4, 2016

jdm commented Oct 13, 2016

g-k commented Oct 15, 2016

jdm commented Oct 16, 2016

bors-servo commented Oct 16, 2016

bors-servo commented Oct 16, 2016

bors-servo commented Oct 16, 2016

servo / servo

Join GitHub today

wpt cookie path tests #11624

wpt cookie path tests #11624

Conversation

g-k commented Jun 5, 2016 • edited by larsbergstrom

KiChjang commented Jun 5, 2016

g-k commented Jun 5, 2016

highfive commented Jun 5, 2016

KiChjang commented Jun 6, 2016

jdm commented Jun 6, 2016

bors-servo commented Jun 6, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 23, 2016

bors-servo commented Jun 30, 2016

bors-servo commented Jul 6, 2016

jdm commented Oct 4, 2016

g-k commented Oct 4, 2016

jdm commented Oct 13, 2016

g-k commented Oct 15, 2016

jdm commented Oct 16, 2016

bors-servo commented Oct 16, 2016

bors-servo commented Oct 16, 2016

bors-servo commented Oct 16, 2016

g-k commented Jun 5, 2016 •

edited by larsbergstrom