Closed
Bug 736764
Opened 13 years ago
Closed 12 years ago
Remove dependency on preferences dialog for tests which only set preferences
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 fixed)
People
(Reporter: whimboo, Assigned: vladmaniac)
References
()
Details
(Whiteboard: [good first bug][mozmill-refactor][qa-])
Attachments
(5 files, 7 obsolete files)
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 |
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•13 years ago
|
||
Exactly that's what we have to do to not be surprised when tenth of tests will be start failing.
(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•13 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.
Comment 5•13 years ago
|
||
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•13 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
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
Status: NEW → ASSIGNED
Comment 12•12 years ago
|
||
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•12 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 14•12 years ago
|
||
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 15•12 years ago
|
||
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)
Comment 16•12 years ago
|
||
(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•12 years ago
|
||
All requests were addressed, excluding tests/functional/testSecurity/testDefaultPhishingEnabled.
Attachment #611446 -
Attachment is obsolete: true
Attachment #614352 -
Flags: review?(dave.hunt)
Reporter | ||
Comment 18•12 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.
Comment 19•12 years ago
|
||
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•12 years ago
|
||
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-
Comment 21•12 years ago
|
||
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•12 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.
Comment 23•12 years ago
|
||
Any update on this?
Comment 24•12 years ago
|
||
(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•12 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•12 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?
Comment 27•12 years ago
|
||
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)
Comment 28•12 years ago
|
||
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•12 years ago
|
||
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-
Comment 30•12 years ago
|
||
(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•12 years ago
|
||
Renamed testMasterPassword to testPreferences_masterPassword.
Attachment #623665 -
Attachment is obsolete: true
Attachment #624026 -
Flags: review?(dave.hunt)
Reporter | ||
Comment 32•12 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 33•12 years ago
|
||
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-
Comment 34•12 years ago
|
||
Tests are moved to testPreferences/ and manifests are updated.
Attachment #624026 -
Attachment is obsolete: true
Attachment #624351 -
Flags: review?(dave.hunt)
Comment 35•12 years ago
|
||
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 36•12 years ago
|
||
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-
Comment 37•12 years ago
|
||
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•12 years ago
|
||
The problem is with testPreferredLanguage not being able to click on "Choose", when selecting the language.
Comment 39•12 years ago
|
||
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.
Reporter | ||
Comment 40•12 years ago
|
||
Remus, please test this patch again now that bug 757340 has been fixed. Does it work?
Comment 41•12 years ago
|
||
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•12 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-
Comment 43•12 years ago
|
||
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•12 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•12 years ago
|
||
Pushed to default: http://hg.mozilla.org/qa/mozmill-tests/rev/194a0e997281
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 46•12 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)
Comment 47•12 years ago
|
||
It seems there were leftovers in restartTests, regarding masterPassword. This patch fixes them.
Attachment #640944 -
Flags: review?(hskupin)
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 48•12 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•12 years ago
|
||
Follow-up landed on default: http://hg.mozilla.org/qa/mozmill-tests/rev/f4d1c91259f5
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 50•12 years ago
|
||
Attachment #641445 -
Flags: review?(hskupin)
Comment 51•12 years ago
|
||
Attachment #641451 -
Flags: review?(hskupin)
Reporter | ||
Updated•12 years ago
|
Attachment #641445 -
Flags: review?(hskupin) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #641451 -
Flags: review?(hskupin) → review+
Reporter | ||
Comment 52•12 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.
Reporter | ||
Updated•12 years ago
|
Comment 53•12 years ago
|
||
Updated so it applies cleanly on esr10.
Attachment #642577 -
Flags: review?(hskupin)
Attachment #642577 -
Flags: review?(dave.hunt)
Reporter | ||
Comment 54•12 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•12 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•12 years ago
|
||
Fixed
Attachment #642577 -
Attachment is obsolete: true
Attachment #643310 -
Flags: review?(hskupin)
Reporter | ||
Updated•12 years ago
|
Attachment #643310 -
Flags: review?(hskupin) → review+
Reporter | ||
Comment 57•12 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•12 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•12 years ago
|
Flags: in-litmus?(vlad.mozbugs) → in-litmus+
Assignee | ||
Comment 59•12 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•12 years ago
|
||
Yes, that's 10.0 as what the ESR is for.
Flags: in-litmus+ → in-litmus?(vlad.mozbugs)
Assignee | ||
Comment 61•12 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•12 years ago
|
Flags: in-litmus?(vlad.mozbugs) → in-litmus+
Updated•12 years ago
|
Whiteboard: [good first bug][mozmill-refactor] → [good first bug][mozmill-refactor][qa-]
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•