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...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: All All
: P4 Trivial
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-10 18:04 PDT by Jason Yan
Modified: 2009-10-19 15:44 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Yan 2009-10-10 18:04:04 PDT
Created attachment 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 Eric Seidel 2009-10-10 19:45:38 PDT
Comment on attachment 40995 [details]
patch against r49292

Seems fine to me, but needs a ChangeLog.  Also whoever wrote this test shoudl probably be CC'd on this bug.
Comment 2 Eric Seidel 2009-10-10 19:46:37 PDT
CCing Dave (the author) and Anders (the reviewer).  Was added by:
http://trac.webkit.org/changeset/35534
Comment 3 Eric Seidel 2009-10-10 19:48:06 PDT
Sorry, I was wrong.  It was added as:
http://trac.webkit.org/changeset/25110
Comment 4 Jason Yan 2009-10-10 20:09:06 PDT
Created attachment 40997 [details]
Updated ChangeLog

Updated ChangeLog and tested against r49422.
Comment 5 Jason Yan 2009-10-10 20:10:02 PDT
Created attachment 40998 [details]
Updated ChangeLog

As a patch this time.
Comment 6 Eric Seidel 2009-10-10 20:21:20 PDT
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 Jason Yan 2009-10-10 20:33:52 PDT
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 Darin Adler 2009-10-12 09:21:57 PDT
Comment on attachment 40998 [details]
Updated ChangeLog

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 Jason Yan 2009-10-12 18:14:39 PDT
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 Jason Yan 2009-10-12 18:18:23 PDT
Created attachment 41078 [details]
Incorporated suggests from Darin.  Patch against r49487.
Comment 11 Yong Li 2009-10-19 08:49:15 PDT
Comment on attachment 41078 [details]
Incorporated suggests from Darin.  Patch against r49487.

Let commit bot land it
Comment 12 Eric Seidel 2009-10-19 11:41:25 PDT
Comment on attachment 41078 [details]
Incorporated suggests from Darin.  Patch against r49487.

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 Jason Yan 2009-10-19 15:13:04 PDT
Created attachment 41456 [details]
Fixed spacing in ChangeLog
Comment 14 Eric Seidel 2009-10-19 15:22:39 PDT
Comment on attachment 41456 [details]
Fixed spacing in ChangeLog

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 WebKit Commit Bot 2009-10-19 15:44:28 PDT
Comment on attachment 41456 [details]
Fixed spacing in ChangeLog

Clearing flags on attachment: 41456

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