585781 - Make Mozmill-test testStartStopPBMode local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: testPrivateBrowsing/testStartStopPBMode.js
Test-page: Use any 1 page from test-files/layout/mozilla*

Comment 1

[u279076]

Attachment: Patch v1 (default)

Comment 2

[Aaron Train [:aaronmt]]

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

># HG changeset patch
># User Anthony Hughes <[e-mail]>
># Date 1281568715 25200
># Node ID 03d1d5848acdecaab48e54e0f92b1076d81e591d
># Parent  59901f2281ede0840e7550269a6609cca8c468fa
>  * Contributor(s):
>  *   Henrik Skupin <[e-mail]>
>- *   Aaron Train <[e-mail]
>+ *   Aaron Train <[e-mail]>
>+ *   Anthony Hughes <[e-mail]>
Thanks.

>   // Wait until all tabs have been finished loading
>-  for each(var localPage in LOCAL_PAGES) {
>-   var elem = new elementslib.ID(controller.tabs.getTab(localPage.index), localPage.id);
>+  for (var i=0; i<LOCAL_TEST_PAGES.length; i++) {
>+   var elem = new elementslib.ID(controller.tabs.getTab(i), LOCAL_TEST_PAGES[i].id);
>    controller.waitForElement(elem, TIMEOUT);
>   }

Nit: Spaces within the declarations and conditions in the for statement.

>-  for (var ii = 0; ii < LOCAL_PAGES.length; ii++) {
>-    var elem = new elementslib.ID(controller.tabs.getTab(ii), LOCAL_PAGES[ii].id);
>+  for (var i=0; i<LOCAL_TEST_PAGES.length; i++) {
>+    var elem = new elementslib.ID(controller.tabs.getTab(i), LOCAL_TEST_PAGES[i].id);
>     controller.waitForElement(elem, TIMEOUT);
>   }

Same here

Comment 3

[u279076]

Attachment: Patch v2 (default)

Comment 4

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

Comment on attachment 465039 [details] [diff] [review]
Patch v2 (default)

>+const LOCAL_TEST_PAGES = [
>+                          {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html', 
>+                           id: 'community'},
>+                          {url: 'about:', 
>+                           id: 'aboutPageList'}
>+                         ];

The indentation has been messed-up.

>+  for each(var page in LOCAL_TEST_PAGES) {

nit: please add a space behind each.

>-  controller.assertJS("subject.tabs.length == " + (LOCAL_PAGES.length + 1),
>+  controller.assertJS("subject.tabs.length == " + (LOCAL_TEST_PAGES.length + 1),
>                       controller);

Please pass in both variables as object members instead of concatenating strings.

Comment 5

[u279076]

Attachment: Patch v3 (default)

nits addressed, also found a couple other assertJS() which could have used the treatment.

Comment 6

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

Comment on attachment 465866 [details] [diff] [review]
Patch v3 (default)

> var setupModule = function(module) {
>+var teardownModule = function(module) {

nit: please remove module.

r=me with that comment addressed.

Comment 7

[u279076]

Attachment: Patch v3 (default) + nits

Comment 8

[u279076]

> Created attachment 465890 [details] [diff] [review]
> Patch v3 (default) + nits

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/72c30a7df39f (default)

Does not cleanly apply to mozilla1.9.2 and mozilla1.9.1, patch forthcoming.

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.2)

(In reply to comment #8)
> > Created attachment 465890 [details] [diff] [review] [details]
> > Patch v3 (default) + nits
> 
> Landed:
> http://hg.mozilla.org/qa/mozmill-tests/rev/72c30a7df39f (default)
> 
> Does not cleanly apply to mozilla1.9.2 and mozilla1.9.1, patch forthcoming.
Done. Applies to 1.9.2 and 1.9.1

Comment 10

[u279076]

(In reply to comment #9)
> Created attachment 477979 [details] [diff] [review]
> Patch v3 - (1.9.2)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/443179c65eec [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/4920f5aa349f [mozilla1.9.1]

Comment 11

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

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.