The default bug view has changed. See this FAQ.

Remove dependency on preferences dialog for tests which only set preferences

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: vladmaniac)

Tracking

unspecified
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [good first bug][mozmill-refactor][qa-], URL)

Attachments

(5 attachments, 7 obsolete attachments)

119.09 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.92 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
120.86 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
121.47 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
149.75 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
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?
(Reporter)

Comment 2

5 years ago
Exactly that's what we have to do to not be surprised when tenth of tests will be start failing.
Whiteboard: [good first bug] → [good first bug][mozmill-refactor]
(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.
(Reporter)

Comment 4

5 years ago
(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.
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
Assignee: nobody → remus.pop
(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?
(Reporter)

Comment 7

5 years ago
Up to you. It was just a proposal.
(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
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.
Created attachment 611446 [details] [diff] [review]
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.
Attachment #611446 - Flags: review?(vlad.mozbugs)
Attachment #611446 - Flags: feedback?(hskupin)
(Assignee)

Comment 11

5 years ago
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
Attachment #611446 - Flags: review?(vlad.mozbugs)
Attachment #611446 - Flags: review?(anthony.s.hughes)
Attachment #611446 - Flags: review+
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 13

5 years ago
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.
Attachment #611446 - Flags: review?(dave.hunt)
Attachment #611446 - Flags: review?(anthony.s.hughes)
Attachment #611446 - Flags: feedback?(hskupin)
Attachment #611446 - Flags: feedback?(anthony.s.hughes)
Comment on attachment 611446 [details] [diff] [review]
patch v1 (all branches)

># HG changeset patch
># User Remus Pop <remus.pop@softvision.ro>
># 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.
Attachment #611446 - Flags: review?(dave.hunt) → review-
Comment on attachment 611446 [details] [diff] [review]
patch v1 (all branches)

Canceling feedback based on Dave's review.
Attachment #611446 - Flags: feedback?(anthony.s.hughes)
(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?
Created attachment 614352 [details] [diff] [review]
patch v2 (default)

All requests were addressed, excluding tests/functional/testSecurity/testDefaultPhishingEnabled.
Attachment #611446 - Attachment is obsolete: true
Attachment #614352 - Flags: review?(dave.hunt)
(Reporter)

Comment 18

5 years ago
(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.
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 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?
Attachment #614352 - Flags: review?(dave.hunt) → review-
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?
(Reporter)

Comment 22

5 years ago
(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.
Any update on this?
(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.
(Reporter)

Comment 25

5 years ago
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?
(Reporter)

Comment 26

5 years ago
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?
Created attachment 623665 [details] [diff] [review]
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.
Attachment #614352 - Attachment is obsolete: true
Attachment #623665 - Flags: review?(dave.hunt)
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 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.
Attachment #623665 - Flags: review?(dave.hunt) → review-
(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.
Created attachment 624026 [details] [diff] [review]
patch v4 (default)

Renamed testMasterPassword to testPreferences_masterPassword.
Attachment #623665 - Attachment is obsolete: true
Attachment #624026 - Flags: review?(dave.hunt)
(Reporter)

Comment 32

5 years ago
(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 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
Attachment #624026 - Flags: review?(dave.hunt) → review-
Created attachment 624351 [details] [diff] [review]
patch v5 (default)

Tests are moved to testPreferences/ and manifests are updated.
Attachment #624026 - Attachment is obsolete: true
Attachment #624351 - Flags: review?(dave.hunt)
Comment on attachment 624351 [details] [diff] [review]
patch v5 (default)

Looks good to me. Re-adding Anthony for feedback.
Attachment #624351 - Flags: review?(dave.hunt)
Attachment #624351 - Flags: review+
Attachment #624351 - Flags: feedback?(anthony.s.hughes)
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.
Attachment #624351 - Flags: feedback?(anthony.s.hughes) → feedback-
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.
The problem is with testPreferredLanguage not being able to click on "Choose", when selecting the language.
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.

Updated

5 years ago
Depends on: 757340
(Reporter)

Comment 40

5 years ago
Remus, please test this patch again now that bug 757340 has been fixed. Does it work?
Created attachment 638340 [details] [diff] [review]
patch v6 (default)

Updated so it applies cleanly now.
Attachment #624351 - Attachment is obsolete: true
Attachment #638340 - Flags: review?(hskupin)
Attachment #638340 - Flags: review?(dave.hunt)
(Reporter)

Comment 42

5 years ago
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.
Attachment #638340 - Flags: review?(hskupin)
Attachment #638340 - Flags: review?(dave.hunt)
Attachment #638340 - Flags: review-
Created attachment 639338 [details] [diff] [review]
patch v7 (default)

Addressed all requests.
Also removed the setting of the pref in testBackgroundScrolling as it matches the default behaviour.
Attachment #638340 - Attachment is obsolete: true
Attachment #639338 - Flags: review?(hskupin)
(Reporter)

Comment 44

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

Looks great. I'm going to land this now.
Attachment #639338 - Flags: review?(hskupin) → review+
(Reporter)

Comment 45

5 years ago
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/194a0e997281
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 46

5 years ago
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.
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → fixed
Flags: in-litmus?(remus.pop)
Created attachment 640944 [details] [diff] [review]
follow-up restart tests v1 (default)

It seems there were leftovers in restartTests, regarding masterPassword. This patch fixes them.
Attachment #640944 - Flags: review?(hskupin)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 48

5 years ago
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.
Attachment #640944 - Attachment description: patch v1 (default) → follow-up restart tests v1 (default)
Attachment #640944 - Flags: review?(hskupin) → review+
(Reporter)

Comment 49

5 years ago
Follow-up landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/f4d1c91259f5
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Created attachment 641445 [details] [diff] [review]
patch v7 (aurora, beta)
Attachment #641445 - Flags: review?(hskupin)
Created attachment 641451 [details] [diff] [review]
patch v7 (release)
Attachment #641451 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Attachment #641445 - Flags: review?(hskupin) → review+
(Reporter)

Updated

5 years ago
Attachment #641451 - Flags: review?(hskupin) → review+
(Reporter)

Comment 52

5 years ago
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.
status-firefox13: affected → fixed
status-firefox14: affected → fixed
(Reporter)

Updated

5 years ago
status-firefox15: affected → fixed
Created attachment 642577 [details] [diff] [review]
patch v7 (esr10)

Updated so it applies cleanly on esr10.
Attachment #642577 - Flags: review?(hskupin)
Attachment #642577 - Flags: review?(dave.hunt)
(Reporter)

Comment 54

5 years ago
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.
Attachment #642577 - Flags: review?(hskupin)
Attachment #642577 - Flags: review?(dave.hunt)
Attachment #642577 - Flags: review+
(Reporter)

Comment 55

5 years ago
(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.
Assignee: remus.pop → vlad.mozbugs
(Assignee)

Comment 56

5 years ago
Created attachment 643310 [details] [diff] [review]
patch v7.1 (esr10)

Fixed
Attachment #642577 - Attachment is obsolete: true
Attachment #643310 - Flags: review?(hskupin)
(Reporter)

Updated

5 years ago
Attachment #643310 - Flags: review?(hskupin) → review+
(Reporter)

Comment 57

5 years ago
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.
Flags: in-litmus?(remus.pop) → in-litmus?(vlad.mozbugs)
(Assignee)

Comment 58

5 years ago
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
(Assignee)

Updated

5 years ago
Flags: in-litmus?(vlad.mozbugs) → in-litmus+
(Assignee)

Comment 59

5 years ago
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
(Reporter)

Comment 60

5 years ago
Yes, that's 10.0 as what the ESR is for.
Flags: in-litmus+ → in-litmus?(vlad.mozbugs)
(Assignee)

Comment 61

5 years ago
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
(Assignee)

Updated

5 years ago
Flags: in-litmus?(vlad.mozbugs) → in-litmus+

Updated

5 years ago
status-firefox-esr10: affected → fixed
Whiteboard: [good first bug][mozmill-refactor] → [good first bug][mozmill-refactor][qa-]
You need to log in before you can comment on or make changes to this bug.