Module: testCookies/testEnableCookies.js Test-page: test-files/cookies/cookie_single.html

cleanup + convert test to make use of local-data

Comment on attachment 467021 [details] [diff] [review] Patch v1 - (default) > controller.assertJS("subject.cookieExists == true", > {cookieExists: cm.cookieExists({host: ".mozilla.org", name: "__utmz", path: "/"})}); Please fix this statement to look something like this: controller.assertJS("subject.cookieExists == true", {cookieExists: cm.cookieExists( {host: ".mozilla.org", name: "__utmz", path: "/"}) });

(In reply to comment #2) The opening brackets should be one line above. We can make it much better readable that way and have much more available space. Do we wanna continue that way? > controller.assertJS("subject.cookieExists == true", { > cookieExists: cm.cookieExists({ > host: ".mozilla.org", > name: "__utmz", path: "/" > }) > });

That works for me.

indent fix

Comment on attachment 471494 [details] [diff] [review] Patch v1.1 - (default) >+var setupModule = function() > { Please fix the brackets for all functions. >+ cm = Cc["@mozilla.org/cookiemanager;1"] > .getService(Ci.nsICookieManager2); Also the indentation here. >- controller.waitForElement(removeCookieButton, gTimeout); >- controller.assertProperty(removeCookieButton, "disabled", false); >+ controller.waitForElement(removeCookieButton, TIMEOUT); >+ controller.assertDOMProperty(removeCookieButton, "disabled", false); Don't switch to assertDOMProperty here. That should be part of the general patch and be done in a single bug. >+ controller.assertJS("subject.cookieExists == true", { >+ cookieExists: cm.cookieExists({ >+ host: ".mozilla.org", >+ name: "__utmz", path: "/" >+ }) >+ }); How does that work? We don't use mozilla.org anymore. This should be localhost. Have you tested your patch? >+ controller.assertJS("subject.cookieCount > 0", { >+ cookieCount: cm.countCookiesFromHost(".mozilla.org") >+ }); Same here.

Fixes from comment #6

Fixes from comment #6

Comment on attachment 475271 [details] [diff] [review] Patch v1.2 - (default) >- // Go to mozilla.org to build a list of cookies >+ // Go to a local test page to build a list of cookies nit: Please remove "a list of cookies". That gives a wrong assumption because the test file only creates on single cookie. > controller.select(historyMode, null, null, "custom"); >- controller.sleep(gDelay); Removing the sleep doesn't break the test? I can remember that in all my testings in the past the sleep was necessary. >+ // Search for cookies from localhost and verify cookies are saved nit: Please update the content too. We only set a single cookie but we do not search. >+ controller.assertJS("subject.cookieExists == true", { >+ cookieExists: cm.cookieExists({ >+ host: "localhost", >+ name: "__utmz", path: "/" >+ }) >+ }); That does really pass? Where has __utmz been set? Also move path to the next line. For the host part please strip the information from the loaded url in the locationbar (see my other review). >+ controller.assertJS("subject.cookieCount > 0", { >+ cookieCount: cm.countCookiesFromHost("localhost") >+ }); We only have one cookie. So we don't need this assertJS call.

> > controller.select(historyMode, null, null, "custom"); > >- controller.sleep(gDelay); > > Removing the sleep doesn't break the test? I can remember that in all my > testings in the past the sleep was necessary. the gDelay value was 0, which deems redundant if we sleep for 0 - test doesn't break. I think you told me in a comment a while back that there's no point to keep 0 second sleeps. > >+ controller.assertJS("subject.cookieExists == true", { > >+ cookieExists: cm.cookieExists({ > >+ host: "localhost", > >+ name: "__utmz", path: "/" > >+ }) > >+ }); > > That does really pass? Where has __utmz been set? Also move path to the next > line. For the host part please strip the information from the loaded url in the > locationbar (see my other review). Yeah it's silly it passes still, not sure of the logic - but thanks I changed it to litmus_1 like you caught in the other bug. Did you want nsIURI.host or window.content.location.hostname?

(In reply to comment #10) > the gDelay value was 0, which deems redundant if we sleep for 0 - test doesn't > break. I think you told me in a comment a while back that there's no point to > keep 0 second sleeps. Then we are fine. > Yeah it's silly it passes still, not sure of the logic - but thanks I changed > it to litmus_1 like you caught in the other bug. Did you want nsIURI.host or > window.content.location.hostname? Lets follow-up there. No need to have this discussion on two bugs.

+ get hostname from loaded page and proper path to local cookie

Comment on attachment 477120 [details] [diff] [review] Patch v1.3 - (default) >+var hostName; What's the reason for the global object? If you want to share it between the test function and the callbacks please use the persisted object as we do in our restart tests. You should clear the object in teardown. Otherwise it looks good. Seems to be one more round.

Comment on attachment 477498 [details] [diff] [review] Patch v1.4 - (default) Woops wrong patch for wrong bug

Uses a persisted object and assigns undefined in the teardown

Comment on attachment 477951 [details] [diff] [review] Patch v1.4 - (default) [persisted object] r=me assumed this patch is tested.

(In reply to comment #16) > Created attachment 477951 [details] [diff] [review] > Patch v1.4 - (default) [persisted object] > > Uses a persisted object and assigns undefined in the teardown Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/ed556b5e6605 [default] http://hg.mozilla.org/qa/mozmill-tests/rev/6e1829b0e5b3 [mozilla1.9.2] Please provide a patch for mozilla1.9.1

(In reply to comment #19) > Created attachment 478037 [details] [diff] [review] > Patch v1.4 - (1.9.1) Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/040f2f9c4db7 [mozilla1.9.1]

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.