736764 - Remove dependency on preferences dialog for tests which only set preferences

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

With Firefox 15 we will get inline preferences and the old preferences dialog will be gone. To be prepared for this change we should already start to update our tests which currently make use of the preferences dialog, but only use it to set some preferences for the real test.

Instead of using the preferences dialog those tests have to use the preferences API for setting the pref.

Comment 1

[u279076]

If I understand correctly, what we need to do is update any existing tests which alter preferences to do so manually (not via UI) and this action should be performed in the Prefs shared module.

Is that correct?

Comment 2

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

Exactly that's what we have to do to not be surprised when tenth of tests will be start failing.

Comment 3

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Exactly that's what we have to do to not be surprised when tenth of tests
> will be start failing.

Thanks Henrik. I've asked Vlad to look into this.

Vlad, a first step would be to identify the tests which set preferences by using the Prefs UI.

Note: the tests which are designed specifically to test the Prefs UI should remain unchanged until this new feature lands in Nightly.

Comment 4

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #3)
> Note: the tests which are designed specifically to test the Prefs UI should
> remain unchanged until this new feature lands in Nightly.

Ideally this should only affect tests under testPreferences. All other subgroups should be changed. I would propose that we do all the work in this bug and submit patches for each individual subgroup.

Comment 5

[Remus Pop (:RemusPop)]

By doing a grep on "openPreferencesDialog" I've identified the following tests. This is what I got at a first glance, but there might be more.
tests/l10n/testAccessKeys/test1.js
tests/l10n/testCropped/test1.js
tests/functional/testCookies/testRemoveAllCookies.js
tests/functional/testCookies/testRemoveCookie.js
tests/functional/testCookies/testEnableCookies.js
tests/functional/testCookies/testDisableCookies.js
tests/functional/testFormManager/testDisableFormManager.js
tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js
tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
tests/functional/testAwesomeBar/testVisibleItemsMax.js
tests/functional/testAwesomeBar/testCheckItemHighlight.js
tests/functional/testSecurity/testSSLDisabledErrorPage.js
tests/functional/testSecurity/testDefaultPhishingEnabled.js
tests/functional/testSecurity/testDefaultSecurityPrefs.js
tests/functional/restartTests/testMasterPassword/test1.js
tests/functional/restartTests/testMasterPassword/test2.js
tests/functional/restartTests/testMasterPassword/test3.js
tests/functional/testTabbedBrowsing/testOpenInForeground.js
tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
tests/functional/testTabbedBrowsing/testOpenInBackground.js
tests/functional/testPopups/testPopupsBlocked.js
tests/functional/testPopups/testPopupsAllowed.js
tests/functional/testPrivateBrowsing/testDisabledPermissions.js
tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js
tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
tests/functional/testPasswordManager/testPasswordNotSaved.js

Comment 6

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #3)
> > Note: the tests which are designed specifically to test the Prefs UI should
> > remain unchanged until this new feature lands in Nightly.
> 
> Ideally this should only affect tests under testPreferences. All other
> subgroups should be changed. I would propose that we do all the work in this
> bug and submit patches for each individual subgroup.

That makes a lot of extra work in my opinion. If we do many smaller patches it's a lot more work in the check-in and testrun verification stage.

If the change is as trivial as removing the preferences dialog callback and manually setting the pref then I think this can be done is one big patch.

Is there a risk that I am not seeing, Henrik?

Comment 7

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

Up to you. It was just a proposal.

Comment 8

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Up to you. It was just a proposal.

Okay, thanks.

Remus, please do this all in one patch -- it will be easier for me. Thanks

Comment 9

[Remus Pop (:RemusPop)]

tests/functional/testAwesomeBar/testCheckItemHighlight.js
tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js
tests/functional/testAwesomeBar/testVisibleItemsMax.js
tests/functional/testFormManager/testDisableFormManager.js
tests/functional/testPopups/testPopupsAllowed.js
tests/functional/testPopups/testPopupsBlocked.js
tests/functional/testPrivateBrowsing/testDisabledPermissions.js
tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js
tests/functional/testSecurity/testSSLDisabledErrorPage.js
tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
tests/functional/testTabbedBrowsing/testOpenInBackground.js
tests/functional/testTabbedBrowsing/testOpenInForeground.js

This are the touched tests. Others opened the cookie manager for example. Let me know if you identify more.

Comment 10

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

Chnages include the creation of a pref constant in every test.
This isn't tested in esr. Please let me know if we need to port it there too.

When we migrate to in content prefs I think it will be useful to have a build a few days before the new pref system gets into Firefox 15 so migration will be easier and without many failures.

Comment 11

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 611446 [details] [diff] [review]
patch v1 (all branches)

+ from me. Thanks for the patch Remus. 

Over to Anthony, and lets see the feedback from Henrik also

Comment 12

[u279076]

I'm holding review until Henrik has a chance to give feedback. My one initial suggestion would be to move an pref CONSTANTS to the respective shared module if they are to be used with multiple tests.

Comment 13

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

Comment on attachment 611446 [details] [diff] [review]
patch v1 (all branches)

Updating review/feedback requests to Dave and Anthony. Dave please check it code-wise while Anthony should make sure that we do not have a regression in the test coverage.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 611446 [details] [diff] [review]
patch v1 (all branches)

># HG changeset patch
># User Remus Pop <[e-mail]>
># Date 1333092202 -10800
># Node ID 5cb7a9708a09493610ee0d912d67f5a873268697
># Parent  f9b73bdfb82fb3122fe5a9b703fe08686d3e7862
>Bug 736764 - Remove dependency on preferences dialog for tests which only set preferences r=vlad.maniac, r=ashughes
>
>diff --git a/tests/functional/testAwesomeBar/testCheckItemHighlight.js b/tests/functional/testAwesomeBar/testCheckItemHighlight.js
>-  // Use preferences dialog to select:
>-  // "When Using the location bar suggest:" History and Bookmarks
>-  prefs.openPreferencesDialog(controller, prefDialogSuggestsCallback);
>+  // Location bar suggests "History and Bookmarks
>+  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 0);

Nit: unclosed quotes in comment

>diff --git a/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js b/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js
>+const PREF_LOCATION_BAR_SUGGEST = "browser.urlbar.default.behavior";
> const TIMEOUT = 5000;

Nit: for consistency with other files, please add a blank line between these constants

>-  // Use preferences dialog to select "When Using the location bar suggest:" History and Bookmarks
>-  prefs.openPreferencesDialog(controller, prefDialogSuggestsCallback);
>+  // Location bar suggests "History and Bookmarks
>+  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 0);

Nit: unclosed quotes in comment

>diff --git a/tests/functional/testAwesomeBar/testVisibleItemsMax.js b/tests/functional/testAwesomeBar/testVisibleItemsMax.js
>+const PREF_LOCATION_BAR_SUGGEST = "browser.urlbar.default.behavior";
>+
> 
> var setupModule = function() {

Nit: Remove one of these empty lines before setupModule

>diff --git a/tests/functional/testSecurity/testSSLDisabledErrorPage.js b/tests/functional/testSecurity/testSSLDisabledErrorPage.js
> const gDelay = 0;
> const gTimeout = 5000;
>+const PREF_SSL_3 = "security.enable_ssl3";
>+const PREF_TLS = "security.enable_tls";

Nit: for consistency with other files, please put the pref constants first, and add a blank line before the delay/timeout constants

>diff --git a/tests/functional/testTabbedBrowsing/testOpenInForeground.js b/tests/functional/testTabbedBrowsing/testOpenInForeground.js
> const localTestFolder = collector.addHttpResource('../../../data/');
>+const PREF_TAB_LOAD_IN_BACKGROUND = "browser.tabs.loadInBackground";

Nit: for consistency with other files, please add a blank line between these constants

There appear to be more instances of the preference dialog that have not been covered with this patch. For example tests/functional/restartTests/testMasterPassword and tests/functional/testSecurity/testDefaultPhishingEnabled.

Comment 15

[u279076]

Comment on attachment 611446 [details] [diff] [review]
patch v1 (all branches)

Canceling feedback based on Dave's review.

Comment 16

[Remus Pop (:RemusPop)]

(In reply to Dave Hunt (:davehunt) from comment #14)

> There appear to be more instances of the preference dialog that have not
> been covered with this patch. For example
> tests/functional/restartTests/testMasterPassword and
Uses the pref dialog to access the setting of master password

> tests/functional/testSecurity/testDefaultPhishingEnabled.
This check to see if "Block reported attack sites" and "Block reported web forgeries" are checked in the prefs. Would you like me to get the pref setting from about:config instead?

Comment 17

[Remus Pop (:RemusPop)]

Attachment: patch v2 (default)

All requests were addressed, excluding tests/functional/testSecurity/testDefaultPhishingEnabled.

Comment 18

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

(In reply to Remus Pop (:RemusPop) from comment #16)
> > There appear to be more instances of the preference dialog that have not
> > been covered with this patch. For example
> > tests/functional/restartTests/testMasterPassword and
> Uses the pref dialog to access the setting of master password

Which settings? As long as we can use API's we can do it via back-end calls. Probably worth another bug because it sounds like it is a lot of work.

> > tests/functional/testSecurity/testDefaultPhishingEnabled.
> This check to see if "Block reported attack sites" and "Block reported web
> forgeries" are checked in the prefs. Would you like me to get the pref
> setting from about:config instead?

If it only checks those checkboxes in the preferences dialog, the test should be moved to testPreferences.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I only gave those two files as examples, there are others that are still using the preferences dialog. It would be good to understand the reasons for all instances that have not been updated. It looks like the list in comment 5 may be complete.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 614352 [details] [diff] [review]
patch v2 (default)

Let's get the remaining uses of preferences dialog fixed in this patch. Can we raise a new bug for the master password test and we'll handle it separately?

Comment 21

[Remus Pop (:RemusPop)]

List of untouched files:

tests/l10n/testAccessKeys/test1.js
tests/l10n/testCropped/test1.js
These two seem to use the DOMWalker, so I'm not sure if we can refactor the tests.

tests/functional/testCookies/testRemoveAllCookies.js
tests/functional/testCookies/testRemoveCookie.js
tests/functional/testCookies/testEnableCookies.js
tests/functional/testCookies/testDisableCookies.js
These tests make use of "Show Cookies" button.


tests/functional/testSecurity/testDefaultPhishingEnabled.js
Test that phising checkboxes are enabled. Henrik's option was to move this to testPreferences

tests/functional/testSecurity/testDefaultSecurityPrefs.js
Test that SSL and TLS are enabled by default. This might also go testPreferences as per Henrik's suggestion

tests/functional/restartTests/testMasterPassword/test1.js
tests/functional/restartTests/testMasterPassword/test2.js
tests/functional/restartTests/testMasterPassword/test3.js
This sets the master password via the master password dialog. No pref is changed when doing this AFAIK.

tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
tests/functional/testPasswordManager/testPasswordNotSaved.js
These tests use the "Show Passwords" button in pane 'Security'.

So, what can we do about these tests?

Comment 22

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

(In reply to Remus Pop (:RemusPop) from comment #21)

> tests/l10n/testAccessKeys/test1.js
> tests/l10n/testCropped/test1.js
> These two seem to use the DOMWalker, so I'm not sure if we can refactor the
> tests.

No, we don't want to refactor any of the l10n tests. Those really test the preference dialog.

> tests/functional/testCookies/testRemoveAllCookies.js
> tests/functional/testCookies/testRemoveCookie.js
> tests/functional/testCookies/testEnableCookies.js
> tests/functional/testCookies/testDisableCookies.js
> These tests make use of "Show Cookies" button.

Can you point us to the appropriate Litmus tests? I kinda would like to know what those are about.

> tests/functional/testSecurity/testDefaultPhishingEnabled.js
> Test that phising checkboxes are enabled. Henrik's option was to move this
> to testPreferences

Doing this would also mean that it has to be done for the Litmus test. Ask the owner of this subgroup if he is ok with it.

> tests/functional/testSecurity/testDefaultSecurityPrefs.js
> Test that SSL and TLS are enabled by default. This might also go
> testPreferences as per Henrik's suggestion

Same as above.

> tests/functional/restartTests/testMasterPassword/test1.js
> tests/functional/restartTests/testMasterPassword/test2.js
> tests/functional/restartTests/testMasterPassword/test3.js
> This sets the master password via the master password dialog. No pref is
> changed when doing this AFAIK.

We have to leave this, right. But I kinda would like to change the folder name to 'testPreferences_masterPassword'.

> tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
> tests/functional/testPasswordManager/testPasswordNotSaved.js
> These tests use the "Show Passwords" button in pane 'Security'.

Same as for the Cookie tests. I would like to see the appropriate Litmus test here.

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Any update on this?

Comment 24

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) from comment #22)

> Can you point us to the appropriate Litmus tests? I kinda would like to know
> what those are about.
> > tests/functional/testCookies/testRemoveAllCookies.js
https://litmus.mozilla.org/show_test.cgi?id=8054

> > tests/functional/testCookies/testRemoveCookie.js
https://litmus.mozilla.org/show_test.cgi?id=8055

> > tests/functional/testCookies/testEnableCookies.js
https://litmus.mozilla.org/show_test.cgi?id=8058

> > tests/functional/testCookies/testDisableCookies.js
https://litmus.mozilla.org/show_test.cgi?id=8053

> Same as for the Cookie tests. I would like to see the appropriate Litmus
> test here.
> > tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
https://litmus.mozilla.org/show_test.cgi?id=50761

> > tests/functional/testPasswordManager/testPasswordNotSaved.js
https://litmus.mozilla.org/show_test.cgi?id=50762
> > These tests use the "Show Passwords" button in pane 'Security'.

Owner of those subgroups is not a bugzilla user so I will email him.

Comment 25

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

Huh? Aren't that Virgil Dicu and Mihaela V who own cookies and password manager? Every owner has to watch the appropriate bugzilla component for ongoing activities. Can you please clarify?

Comment 26

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

We have to get an update here. There was no work happening on this bug in the last 10 days. Please try to get this patch updated appropriately asap. Also when will inline preferences will be turned on, given that those are preffed of at the moment?

Comment 27

[Remus Pop (:RemusPop)]

Attachment: patch v3 (default)

Updated to apply cleanly on default. Moved testDefaultPhishingEnabled.js and testDefaultSecurityPrefs.js to /testPreferences. Litmus will be updated when the patch will go live.

Comment 28

[Remus Pop (:RemusPop)]

Bug 738797 handles the enabling of the inContent prefs but as I can see it depends on UI polishing of the prefs. The feature is scheduled to go live on FF 15.
right now the new pref system is disabled by default in Nightly.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 623665 [details] [diff] [review]
patch v3 (default)

Could you answer the remaining questions from comment 22? If necessary please raise additional bugs to take care of the remaining items.

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Remus Pop (:RemusPop) from comment #24)
> (In reply to Henrik Skupin (:whimboo) from comment #22)
> 
> > > tests/functional/testCookies/testRemoveAllCookies.js
> 
> https://litmus.mozilla.org/show_test.cgi?id=8054
> 
> > > tests/functional/testCookies/testRemoveCookie.js
> 
> https://litmus.mozilla.org/show_test.cgi?id=8055
> 
> > > tests/functional/testCookies/testEnableCookies.js
> 
> https://litmus.mozilla.org/show_test.cgi?id=8058
> 
> > > tests/functional/testCookies/testDisableCookies.js
> 
> https://litmus.mozilla.org/show_test.cgi?id=8053
> 
> > > tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
> 
> https://litmus.mozilla.org/show_test.cgi?id=50761
> 
> > > tests/functional/testPasswordManager/testPasswordNotSaved.js
> 
> https://litmus.mozilla.org/show_test.cgi?id=50762

All of these tests open the preferences dialog, and so will fail once the preferences are inline. I understand that we can't use the preferences API for these tests.

Henrik: Are you aware of any other APIs we could use? Should we raise separate bugs for these tests? At very least we would need to update them to work with the inline preferences.

> > tests/functional/restartTests/testMasterPassword/test1.js
> > tests/functional/restartTests/testMasterPassword/test2.js
> > tests/functional/restartTests/testMasterPassword/test3.js
> > This sets the master password via the master password dialog. No pref is
> > changed when doing this AFAIK.
> 
> We have to leave this, right. But I kinda would like to change the folder name to 'testPreferences_masterPassword'.

Please update this folder name as suggested.

> > > There appear to be more instances of the preference dialog that have not
> > > been covered with this patch. For example
> > > tests/functional/restartTests/testMasterPassword and
> > Uses the pref dialog to access the setting of master password
> 
> Which settings? As long as we can use API's we can do it via back-end calls. Probably worth another bug because it sounds like it is a lot of work.

Please raise a separate bug for finding a more appropriate API for this test to use.

Comment 31

[Remus Pop (:RemusPop)]

Attachment: patch v4 (default)

Renamed testMasterPassword to testPreferences_masterPassword.

Comment 32

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

(In reply to Dave Hunt (:davehunt) from comment #30)
> All of these tests open the preferences dialog, and so will fail once the
> preferences are inline. I understand that we can't use the preferences API
> for these tests.

Given that we are testing the ui of the preferences dialog and not only setting some prefs we should move all those tests under testPreferences. Once inline preferences are enabled there should be equivalent windows we can use. Beside that we should make sure if we really need all of those tests.

> Henrik: Are you aware of any other APIs we could use? Should we raise
> separate bugs for these tests? At very least we would need to update them to
> work with the inline preferences.

We don't want to update those now. Let us simply move those tests to the testPreferences folder for now, so that we have all the tests together we have to update for inline prefs. We will need follow-up bugs for their implementation change.

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 624026 [details] [diff] [review]
patch v4 (default)

As mentioned in comment 32, could you please update the patch to move the following tests to the testPreferences folder:

tests/functional/testCookies/testRemoveAllCookies.js
tests/functional/testCookies/testRemoveCookie.js
tests/functional/testCookies/testEnableCookies.js
tests/functional/testCookies/testDisableCookies.js
tests/functional/testPasswordManager/testPasswordSavedAndDeleted.js
tests/functional/testPasswordManager/testPasswordNotSaved.js

Comment 34

[Remus Pop (:RemusPop)]

Attachment: patch v5 (default)

Tests are moved to testPreferences/ and manifests are updated.

Comment 35

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 624351 [details] [diff] [review]
patch v5 (default)

Looks good to me. Re-adding Anthony for feedback.

Comment 36

[u279076]

Comment on attachment 624351 [details] [diff] [review]
patch v5 (default)

Review of attachment 624351 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks okay; just a couple of nits to address to make the code a bit easier to understand.

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +35,5 @@
>   * Check matched awesomebar items are highlighted.
>   */
>  var testCheckItemHighlight = function() {
> +  // Location bar suggests "History and Bookmarks"
> +  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 0);

This comment is a bit confusing, I would prefer it to read something like "Enable 'history and bookmarks' results in location bar searches".

::: tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
@@ +34,5 @@
>   *
>   */
>  var testFaviconInAutoComplete = function() {
> +  // Location bar suggests "History"
> +  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 1);

This comment is a bit confusing, I would prefer it to read something like "Enable 'history' results in location bar searches".

::: tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js
@@ +35,5 @@
>   * Check history and bookmarked (done in testStarInAutocomplete()) items appear in autocomplete list.
>   */
>  var testSuggestHistoryAndBookmarks = function() {
> +  // Location bar suggests "History and Bookmarks"
> +  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 0);

This comment is a bit confusing, I would prefer it to read something like "Enable 'history and bookmarks' results in location bar searches".

::: tests/functional/testFormManager/testDisableFormManager.js
@@ +29,5 @@
>  }
>  
>  var testToggleFormManager = function() {
> +  // Do not save form and search history
> +  prefs.preferences.setPref(PREF_SAVE_FORM_SEARCH_HISTORY, false);

I would prefer something like "Disable saving form and search history"

::: tests/functional/testTabbedBrowsing/testOpenInBackground.js
@@ +33,5 @@
>    tabBrowser.closeAllTabs();
>  }
>  
>  var testOpenInBackgroundTab = function() {
> +  prefs.preferences.setPref(PREF_TAB_LOAD_IN_BACKGROUND, true);

Should add a comment indicating what this pref does.

::: tests/functional/testTabbedBrowsing/testOpenInForeground.js
@@ +36,5 @@
>  }
>  
>  var testOpenInForegroundTab = function()
>  {
> +  prefs.preferences.setPref(PREF_TAB_LOAD_IN_BACKGROUND, false);

Should add a comment indicating what this pref does.

Comment 37

[Remus Pop (:RemusPop)]

The patch is ready but I cannot upload it because it introduces a regression in the testrun. The problem is with testPrefferedLanguage which complains about a modal-dialog. So at a quick glance, testPasswordSavedAndDeleted is the one to cause it. So if you want to try it out, leave only those two in the folder and run the folder as the test file.
Always reproduces on Mac with Nightly.

Comment 38

[Remus Pop (:RemusPop)]

The problem is with testPreferredLanguage not being able to click on "Choose", when selecting the language.

Comment 39

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Could you raise a new bug for the issue and make it a blocker for this bug and continue with your investigation? It sounds like an earlier test may not be cleaning up after itself adequately.

Comment 40

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

Remus, please test this patch again now that bug 757340 has been fixed. Does it work?

Comment 41

[Remus Pop (:RemusPop)]

Attachment: patch v6 (default)

Updated so it applies cleanly now.

Comment 42

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

Comment on attachment 638340 [details] [diff] [review]
patch v6 (default)

> var testCheckItemHighlight = function() {
>-  // Use preferences dialog to select:
>-  // "When Using the location bar suggest:" History and Bookmarks
>-  prefs.openPreferencesDialog(controller, prefDialogSuggestsCallback);
>+  // Location bar suggests "History and Bookmarks"
>+  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 0);

Setting the preference has to be done in setupModule not in the test, because it's simply not part of it. This applies to all modified tests. 

>-var testRemoveAllCookies = function() {
>-  // Go to mozilla.org to build a list of cookies
>-  controller.open("http://www.mozilla.org/");
>-  controller.waitForPageLoad();
>-
>-  controller.open("http://www.google.com/");
>-  controller.waitForPageLoad();

This test has been updated yesterday so the patch doesn't apply anymore.

>+++ b/tests/functional/testSecurity/testSSLDisabledErrorPage.js
>-  prefs.preferences.clearUserPref("security.enable_ssl3");
>-  prefs.preferences.clearUserPref("security.enable_tls");
>+  prefs.preferences.clearUserPref(PREF_SSL_3);
>+  prefs.preferences.clearUserPref(PREF_TLS);
>   
>   // XXX: Bug 513129
>   //      Re-enable Keep-alive connections
>   prefs.preferences.clearUserPref("network.http.keep-alive");

Please create a const for this last preference.

>+++ b/tests/functional/testTabbedBrowsing/testOpenInForeground.js
>-  prefs.openPreferencesDialog(controller, prefDialogCallback);
>+  prefs.preferences.setPref(PREF_TAB_LOAD_IN_BACKGROUND, false);

What is the default value? Whether it's for this test or open in background. The appropriate test shouldn't modify the preference. We should remove the lines if possible.

Comment 43

[Remus Pop (:RemusPop)]

Attachment: patch v7 (default)

Addressed all requests.
Also removed the setting of the pref in testBackgroundScrolling as it matches the default behaviour.

Comment 44

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

Comment on attachment 639338 [details] [diff] [review]
patch v7 (default)

Looks great. I'm going to land this now.

Comment 45

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

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/194a0e997281

Comment 46

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

I think we should backport this patch, to make it easier for us to transition over to Mozmill 2 without having the rewriting patches for backporting.

Also we have to update the Litmus tests to fix the location of the Mozmill tests.

Comment 47

[Remus Pop (:RemusPop)]

Attachment: follow-up restart tests v1 (default)

It seems there were leftovers in restartTests, regarding masterPassword. This patch fixes them.

Comment 48

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

Comment on attachment 640944 [details] [diff] [review]
follow-up restart tests v1 (default)

Thanks Remus. Please follow up with a combined patch for older branches.

Comment 49

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

Follow-up landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/f4d1c91259f5

Comment 50

[Remus Pop (:RemusPop)]

Attachment: patch v7 (aurora, beta)

Comment 51

[Remus Pop (:RemusPop)]

Attachment: patch v7 (release)

Comment 52

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

http://hg.mozilla.org/qa/mozmill-tests/rev/d78e629ac726 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/14e2273d1209 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/1d77aef4c282 (release)

So now only esr10 is missing.

Comment 53

[Remus Pop (:RemusPop)]

Attachment: patch v7 (esr10)

Updated so it applies cleanly on esr10.

Comment 54

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

Comment on attachment 642577 [details] [diff] [review]
patch v7 (esr10)

This patch doesn't (at least) update the include line of the restart tests for the moved master password test. Please fix that.

Comment 55

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

(In reply to Henrik Skupin (:whimboo) from comment #54)
> This patch doesn't (at least) update the include line of the restart tests
> for the moved master password test. Please fix that.

We still need this. Given that Remus is away, Vlad please come up with a backport ASAP. Thanks.

Comment 56

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v7.1 (esr10)

Fixed

Comment 57

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

Thanks Vlad. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/c809493dcbba

Can you please take care of the appropriate litmus tests and update the changed location of the moved tests? Thanks.

Comment 58

[Maniac Vlad Florin (:vladmaniac)]

Litmus updates for: 

* Firefox Aurora branch: 
https://litmus.mozilla.org/show_test.cgi?id=15526
https://litmus.mozilla.org/show_test.cgi?id=15780
https://litmus.mozilla.org/show_test.cgi?id=15519
https://litmus.mozilla.org/show_test.cgi?id=15524
https://litmus.mozilla.org/show_test.cgi?id=15520
https://litmus.mozilla.org/show_test.cgi?id=15521
https://litmus.mozilla.org/show_test.cgi?id=15639
https://litmus.mozilla.org/show_test.cgi?id=15638
https://litmus.mozilla.org/show_test.cgi?id=15754

* Firefox 14 branch:
https://litmus.mozilla.org/show_test.cgi?id=64127
https://litmus.mozilla.org/show_test.cgi?id=63866
https://litmus.mozilla.org/show_test.cgi?id=63871
https://litmus.mozilla.org/show_test.cgi?id=63867
https://litmus.mozilla.org/show_test.cgi?id=63868
https://litmus.mozilla.org/show_test.cgi?id=63986
https://litmus.mozilla.org/show_test.cgi?id=63985
https://litmus.mozilla.org/show_test.cgi?id=64101
https://litmus.mozilla.org/show_test.cgi?id=63873

* Firefox 13 branch:
https://litmus.mozilla.org/show_test.cgi?id=55773
https://litmus.mozilla.org/show_test.cgi?id=55512
https://litmus.mozilla.org/show_test.cgi?id=55517
https://litmus.mozilla.org/show_test.cgi?id=55513
https://litmus.mozilla.org/show_test.cgi?id=55632
https://litmus.mozilla.org/show_test.cgi?id=55514
https://litmus.mozilla.org/show_test.cgi?id=55621
https://litmus.mozilla.org/show_test.cgi?id=55747
https://litmus.mozilla.org/show_test.cgi?id=55519

Comment 59

[Maniac Vlad Florin (:vladmaniac)]

I could not find an esr branch in Litmus, assuming is Firefox 10? 
If it is so and we need updating there as well, please let me know. Thanks

Comment 60

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

Yes, that's 10.0 as what the ESR is for.

Comment 61

[Maniac Vlad Florin (:vladmaniac)]

Updated litmus testcases for Firefox 10 (mozilla-esr10 branch) 

https://litmus.mozilla.org/show_test.cgi?id=40878
https://litmus.mozilla.org/show_test.cgi?id=40737
https://litmus.mozilla.org/show_test.cgi?id=40736
https://litmus.mozilla.org/show_test.cgi?id=40619
https://litmus.mozilla.org/show_test.cgi?id=40618
https://litmus.mozilla.org/show_test.cgi?id=40617
https://litmus.mozilla.org/show_test.cgi?id=40622
https://litmus.mozilla.org/show_test.cgi?id=40852
https://litmus.mozilla.org/show_test.cgi?id=40624

I think we are alright now