Summary: | LayoutTests/fast/cookies/local-file-can-set-cookies.html fails when cookies for document already exist | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Yan <tailofthesun> | ||||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Trivial | CC: | andersca, aroben, commit-queue, darin, ddkilzer, eric, kmccullough | ||||||||||||
Priority: | P4 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Jason Yan
2009-10-10 18:04:04 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. CCing Dave (the author) and Anders (the reviewer). Was added by: http://trac.webkit.org/changeset/35534 Sorry, I was wrong. It was added as: http://trac.webkit.org/changeset/25110 Created attachment 40997 [details] Updated ChangeLog Updated ChangeLog and tested against r49422. Created attachment 40998 [details]
Updated ChangeLog
As a patch this time.
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? 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 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.
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. Created attachment 41078 [details] Incorporated suggests from Darin. Patch against r49487. Comment on attachment 41078 [details] Incorporated suggests from Darin. Patch against r49487. Let commit bot land it 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. Created attachment 41456 [details]
Fixed spacing in ChangeLog
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 on attachment 41456 [details] Fixed spacing in ChangeLog Clearing flags on attachment: 41456 Committed r49819: <http://trac.webkit.org/changeset/49819> All reviewed patches have been landed. Closing bug. |