Bug 30281 – LayoutTests/fast/cookies/local-file-can-set-cookies.html fails when cookies for document already exist

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jason Yan]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

CCing Dave (the author) and Anders (the reviewer).  Was added by:
http://trac.webkit.org/changeset/35534

[Eric Seidel (no email)]

Sorry, I was wrong.  It was added as:
http://trac.webkit.org/changeset/25110

[Jason Yan]

Created attachment 40997 [details]
Updated ChangeLog

Updated ChangeLog and tested against r49422.

[Jason Yan]

Created attachment 40998 [details]
Updated ChangeLog

As a patch this time.

[Eric Seidel (no email)]

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?

[Jason Yan]

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."

[Darin Adler]

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.

[Jason Yan]

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.

[Jason Yan]

Created attachment 41078 [details]
Incorporated suggests from Darin.  Patch against r49487.

[Yong Li]

Comment on attachment 41078 [details]
Incorporated suggests from Darin.  Patch against r49487.

Let commit bot land it

[Eric Seidel (no email)]

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.

[Jason Yan]

Created attachment 41456 [details]
Fixed spacing in ChangeLog

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

Comment on attachment 41456 [details]
Fixed spacing in ChangeLog

Clearing flags on attachment: 41456

Committed r49819: <http://trac.webkit.org/changeset/49819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.