585756 - Make Mozmill-test testEnableCookies local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

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

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

cleanup + convert test to make use of local-data

Comment 2

[u279076]

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: "/"})
                    });

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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: "/"
>   })
> });

Comment 4

[u279076]

That works for me.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - (default)

indent fix

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v1.2 - (default)

Fixes from comment #6

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Patch v1.2 - (default)

Fixes from comment #6

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 10

[Aaron Train [:aaronmt]]

> >   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?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 12

[Aaron Train [:aaronmt]]

Attachment: Patch v1.3 - (default)

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 14

[Aaron Train [:aaronmt]]

Attachment: Patch v1.4 - (default)

Comment 15

[Aaron Train [:aaronmt]]

Comment on attachment 477498 [details] [diff] [review]
Patch v1.4 - (default)

Woops wrong patch for wrong bug

Comment 16

[Aaron Train [:aaronmt]]

Attachment: Patch v1.4 - (default) [persisted object]

Uses a persisted object and assigns undefined in the teardown

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 477951 [details] [diff] [review]
Patch v1.4 - (default) [persisted object]

r=me assumed this patch is tested.

Comment 18

[u279076]

(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

Comment 19

[Aaron Train [:aaronmt]]

Attachment: Patch v1.4 - (1.9.1)

Comment 20

[u279076]

(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]

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+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.