Bug 30281 - LayoutTests/fast/cookies/local-file-can-set-cookies.html fails when cookies for document already exist
: LayoutTests/fast/cookies/local-file-can-set-cookies.html fails when cookies f...
: WebKit
Tools / Tests
: 528+ (Nightly build)
: All All
: P4 Trivial
Assigned To:
  Show dependency treegraph
Reported: 2009-10-10 18:04 PST by
Modified: 2009-10-19 15:44 PST (History)

patch against r49292 (1.24 KB, patch)
2009-10-10 18:04 PST, Jason Yan
eric: review-
Review Patch | Details | Formatted Diff | Diff
Updated ChangeLog (1.94 KB, application/octet-stream)
2009-10-10 20:09 PST, Jason Yan
no flags Details
Updated ChangeLog (1.94 KB, patch)
2009-10-10 20:10 PST, Jason Yan
darin: review+
Review Patch | Details | Formatted Diff | Diff
Incorporated suggests from Darin. Patch against r49487. (2.38 KB, patch)
2009-10-12 18:18 PST, Jason Yan
darin: review+
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Fixed spacing in ChangeLog (2.38 KB, patch)
2009-10-19 15:13 PST, Jason Yan
no flags Review Patch | Details | Formatted Diff | Diff


You need to log in before you can comment on or make changes to this bug.

Description From 2009-10-10 18:04:04 PST
Created an attachment (id=40995) [details]
patch against r49292

Tested on r49292, LayoutTests/fast/cookies/local-file-can-set-cookies-expected.txt will fail if there are other local file cookies already set on the path the test is being run from.  On my machine, the path was "/" and there were other cookies set already.  This results in a failure because the test checks document.cookie to make sure "ppkcookie1=testcookie" is the only cookie set.  The attached patch changes this to check to use document.cookie.indexOf instead of the equality of document.cookie.
------- Comment #1 From 2009-10-10 19:45:38 PST -------
(From update of attachment 40995 [details])
Seems fine to me, but needs a ChangeLog.  Also whoever wrote this test shoudl probably be CC'd on this bug.
------- Comment #2 From 2009-10-10 19:46:37 PST -------
CCing Dave (the author) and Anders (the reviewer).  Was added by:
------- Comment #3 From 2009-10-10 19:48:06 PST -------
Sorry, I was wrong.  It was added as:
------- Comment #4 From 2009-10-10 20:09:06 PST -------
Created an attachment (id=40997) [details]
Updated ChangeLog

Updated ChangeLog and tested against r49422.
------- Comment #5 From 2009-10-10 20:10:02 PST -------
Created an attachment (id=40998) [details]
Updated ChangeLog

As a patch this time.
------- Comment #6 From 2009-10-10 20:21:20 PST -------
I believe that DumpRenderTree is supposed to clear cookies between test runs, or at least store cookies separately from any system cookies.  This may just be a bug in Qt's DumpRenderTree, or whichever you're using?
------- Comment #7 From 2009-10-10 20:33:52 PST -------
I'm running this test on Mac OS X 10.6.1.  I can reproduce the issue without the patch with the following steps:

1) Create cookie in the same test case (e.g., document.cookie = 'test=broken; expires=Fri, 1 Jan 2010 01:01:01 UTC';)
2) Run test case, expect failure.
3) Remove test cookie from test case.
4) Re-run test case, test still fails with "FAIL document.cookie should be ppkcookie1=testcookie. Was ppkcookie1=testcookie; test=broken."
------- Comment #8 From 2009-10-12 09:21:57 PST -------
(From update of attachment 40998 [details])
This is OK, but if we're going to change the test like this I also suggest that the test do this before setting document.cookie:

    shouldBe("document.cookie.indexOf('ppkcookie1=testcookie')", "-1");

To make sure we are actually setting the cookie. I'll do r=me like this, but you could consider my suggestion.

I also suggest making per-file comments in change log.
------- Comment #9 From 2009-10-12 18:14:39 PST -------
I agree that the test should make sure the cookie isn't already set, however the current cookie has an expiration time of 60 seconds, which can make the test fail if run the test multiple times before the cookie has expired yet.  I've also changed the behavior so that the cookie is set only for the duration of the session.  I'll attach a new patch with your suggestions.
------- Comment #10 From 2009-10-12 18:18:23 PST -------
Created an attachment (id=41078) [details]
Incorporated suggests from Darin.  Patch against r49487.
------- Comment #11 From 2009-10-19 08:49:15 PST -------
(From update of attachment 41078 [details])
Let commit bot land it
------- Comment #12 From 2009-10-19 11:41:25 PST -------
(From update of attachment 41078 [details])
The ChangeLog has a leading space, causing the commit-queue to fail (silently).  Fixing the commit-queue, but this Change will need a new ChagneLog.
------- Comment #13 From 2009-10-19 15:13:04 PST -------
Created an attachment (id=41456) [details]
Fixed spacing in ChangeLog
------- Comment #14 From 2009-10-19 15:22:39 PST -------
(From update of attachment 41456 [details])
Normally you would need to mark this r=? and cq=? in order for anyone to see this and land it.  I just happened to open the bug this afternoon.
------- Comment #15 From 2009-10-19 15:44:28 PST -------
(From update of attachment 41456 [details])
Clearing flags on attachment: 41456

Committed r49819: <http://trac.webkit.org/changeset/49819>
------- Comment #16 From 2009-10-19 15:44:32 PST -------
All reviewed patches have been landed.  Closing bug.