585733 - Make Mozmill-test testDisableCookies local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

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

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

cleanup + converts test to make use of local-data

Comment 2

[u279076]

Comment on attachment 467024 [details] [diff] [review]
Patch v1 - (default)

>+  cm = Cc["@mozilla.org/cookiemanager;1"]
>+          .getService(Ci.nsICookieManager2);
>   cm.removeAll();

Please move the . to the end of the first line and indent getService 2 spaces from Cc.

Adding feedback? for Henrik.  Henrik, I'm thinking we shouldn't have any code like this in tests.  We should move this to an API (testCookiesAPI in this case).  Even if the API file is only a few lines, it can be built out as we go.  I don't think we should burden our community with having to know this stuff. Could/should we add this to the refactor work?

Comment 3

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

(In reply to comment #2)
> >+  cm = Cc["@mozilla.org/cookiemanager;1"]
> >+          .getService(Ci.nsICookieManager2);
> >   cm.removeAll();
> 
> Please move the . to the end of the first line and indent getService 2 spaces
> from Cc.

Please don't indent. Should start at the same column.

> Adding feedback? for Henrik.  Henrik, I'm thinking we shouldn't have any code
> like this in tests.  We should move this to an API (testCookiesAPI in this
> case).  Even if the API file is only a few lines, it can be built out as we go.
>  I don't think we should burden our community with having to know this stuff.
> Could/should we add this to the refactor work?

Makes sense. I wouldn't wait for the refactoring but go ahead. File a new bug which adds the CookieAPI and the functions we need. Further all tests which make use of it should be updated too.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - (default)

fixes indentation

Takes Anthony's comment about the . at the end, and Henrik's about two spaces from the margin. If this is still wrong, you guys need to come to agreement

Comment 5

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

With no indentation I meant no 2 more blanks. Sorry. See the following code:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#267

Comment 6

[u279076]

(In reply to comment #5)
> With no indentation I meant no 2 more blanks. Sorry. See the following code:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#267

Yes, Aaron, follow this example.

FYI, if you ever have any formatting questions, always assume the Mozilla Style guidelines.  You can refer to source code on MXR for examples.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v1.2 - (default)

Sold. Using the example.

Comment 8

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

Comment on attachment 471557 [details] [diff] [review]
Patch v1.2 - (default)

This test will always pass due to the missing update of the following line:

 controller.assertJS("subject.cookieCount == 0",
                      {cookieCount : cm.countCookiesFromHost(".mozilla.org")});

Comment 9

[Aaron Train [:aaronmt]]

Good catch. The cookie preferences manager for the cookie we create in cookie_single.html, has no specified domain nor a comparable path, i.e., (/Users/Aaron/). Can we use cookieExists to check for a blank domain and none specific path for this local cookie we create?

Comment 10

[Aaron Train [:aaronmt]]

Attachment: Patch v1.3 - [localhost] - (default)

Scratch that above comment, was thinking locally instead of localhost. 

Patch changes the assert to check the count of the cookies from host localhost

Comment 11

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

Comment on attachment 474823 [details] [diff] [review]
Patch v1.3 - [localhost] - (default)

>-  // Search for a cookie from mozilla.org and verify cookies are not saved
>+  // Search for a cookie from localhost and verify cookies are not saved
>   controller.assertJS("subject.cookieCount == 0",
>-                      {cookieCount : cm.countCookiesFromHost(".mozilla.org")});
>+                      {cookieCount : cm.countCookiesFromHost("localhost")});

I don't like that we hard-code the value. Please grab the hostname directly from the url in the locationbar after the test page has been loaded. There is an API function you can use. It's fairly straight forward.

Comment 12

[Aaron Train [:aaronmt]]

You talking about nsIURI.host or simply window.content.location.hostname?

Comment 13

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

Whatever makes it easier.

Comment 14

[Aaron Train [:aaronmt]]

Attachment: Patch v1.4 - [fix from comment 11] - (default)

Comment 15

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

Comment on attachment 477124 [details] [diff] [review]
Patch v1.4 - [fix from comment 11] - (default)

Same as for the other cookie test. Please use persisted to store values which are shared between functions.

In the future only update one of the tests which need the same updates. It will lower the amount of review work.

Comment 16

[Aaron Train [:aaronmt]]

Attachment: Patch v1.5 - (default)

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v1.5 - (default)

Please let this be the last one :D

Comment 18

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

Comment on attachment 477504 [details] [diff] [review]
Patch v1.5 - (default)

>+  persisted.hostName = {};

The last part you will have to update is that line. Please assign 'undefined' otherwise we have an empty object lingering around for the remaining tests.

With that change r=me.

Comment 19

[Aaron Train [:aaronmt]]

Attachment: Patch v1.5 - (default + teardown change)

Comment 20

[u279076]

(In reply to comment #19)
> Created attachment 477953 [details] [diff] [review]
> Patch v1.5 - (default + teardown change)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9a2c4c472264 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/d1e39d7472f8 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/c1f94b26ce1c [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.