Reuse of profile causes add-on tests to fail with "Category has been changed"

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: andrei)

Tracking

unspecified

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed, firefox27 fixed, firefox28 fixed, firefox-esr17 fixed, firefox-esr24 fixed)

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
This is a general failure in our tests in teardown. None of the current tests in the repository are resetting the last selected add-on manager category to the discovery pane.

That means if a test was using the extension category and the next test installs a new extension, and want to switch to that category, it is already selected and we time out.

What has to be done here is to reset the selected category for the add-on manager.

This bug should be fixed as quick as possible. Who can take this?
(Reporter)

Comment 1

5 years ago
To reproduce that just run 'mozmill --profile test -b %path% -m %manifest_to_tests'.
(Assignee)

Comment 2

5 years ago
I'll take this since this is the same issue (or at least partly) the problem I am seeing in bug 939690. I can reproduce that issue with a profile set as mentioned in comment 1
Assignee: nobody → andrei.eftimie
Blocks: 939690
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 8338475 [details] [diff] [review]
1.patch

I think I got all of them.

I'll be posting some testrun results shortly
Attachment #8338475 - Flags: review?(hskupin)
Attachment #8338475 - Flags: review?(andreea.matei)
(Assignee)

Comment 4

5 years ago
Created attachment 8338477 [details] [diff] [review]
1.patch

Missed the last change I did from the previous patch
Attachment #8338475 - Attachment is obsolete: true
Attachment #8338475 - Flags: review?(hskupin)
Attachment #8338475 - Flags: review?(andreea.matei)
Attachment #8338477 - Flags: review?(hskupin)
Attachment #8338477 - Flags: review?(andreea.matei)
(Assignee)

Comment 5

5 years ago
Created attachment 8338488 [details] [diff] [review]
2.patch

Needed an update after bug 939690 has landed.

We're now almost green on functional:
http://mozmill-crowd.blargon7.com/#/functional/report/456bebe92845279408c15c03e8ec56a0

and almost green on remote (only bug 940232 seems to be left):
http://mozmill-crowd.blargon7.com/#/remote/report/456bebe92845279408c15c03e8ee4962
Attachment #8338477 - Attachment is obsolete: true
Attachment #8338477 - Flags: review?(hskupin)
Attachment #8338477 - Flags: review?(andreea.matei)
Attachment #8338488 - Flags: review?(hskupin)
Attachment #8338488 - Flags: review?(andreea.matei)
(Reporter)

Comment 6

5 years ago
Comment on attachment 8338488 [details] [diff] [review]
2.patch

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

::: firefox/tests/functional/restartTests/testAddons_changeTheme/test3.js
@@ +23,5 @@
>    delete persisted.theme;
>  
> +  aModule.addonsManager.setCategory({
> +    category: aModule.addonsManager.getCategoryById({id: "discover"})
> +  });

I'm against the ui path in teardown which can always fail. Use the back-end API instead please.
Attachment #8338488 - Flags: review?(hskupin)
Attachment #8338488 - Flags: review?(andreea.matei)
Attachment #8338488 - Flags: review-
(Assignee)

Comment 7

5 years ago
Created attachment 8339128 [details] [diff] [review]
3.patch

I've changed to use the extensions.ui.lastCategory preference to reset the Addons Manager default category.

I've also done 2 small improvements:

1) changed the name of the testAddons_InstallAddonWithoutEULA test
(it had a wrong test name)
2) I've removed an unused pref from testAddons_installFromFTP/test2.js
Attachment #8338488 - Attachment is obsolete: true
Attachment #8339128 - Flags: review?(hskupin)
Attachment #8339128 - Flags: review?(andreea.matei)
(Reporter)

Comment 9

5 years ago
Comment on attachment 8339128 [details] [diff] [review]
3.patch

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

This looks way better. Just one more thing you will have to do. See my inline comment.

::: firefox/tests/remote/restartTests/testAddons_installFromFTP/test2.js
@@ +9,5 @@
>  var {assert} = require("../../../../../lib/assertions");
>  var prefs = require("../../../../lib/prefs");
>  var tabs = require("../../../../lib/tabs");
>  
> +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";

Please don't get rid of the delay pref. It's necessary in here. Somehow the line to reset the user pref got removed. Can you please add it instead?
Attachment #8339128 - Flags: review?(hskupin)
Attachment #8339128 - Flags: review?(andreea.matei)
Attachment #8339128 - Flags: review-
(Assignee)

Comment 10

5 years ago
Created attachment 8339138 [details] [diff] [review]
4.patch

Updated the pref for installFromFTP

Same remote result: http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a03ca67
Attachment #8339128 - Attachment is obsolete: true
Attachment #8339138 - Flags: review?(hskupin)
Attachment #8339138 - Flags: review?(andreea.matei)
(Reporter)

Comment 11

5 years ago
Comment on attachment 8339138 [details] [diff] [review]
4.patch

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

Looks fine. Please get it landed.
Attachment #8339138 - Flags: review?(hskupin)
Attachment #8339138 - Flags: review?(andreea.matei)
Attachment #8339138 - Flags: review+
(Assignee)

Comment 12

5 years ago
Comment on attachment 8339138 [details] [diff] [review]
4.patch

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a0e66c124bc0 (default)
Attachment #8339138 - Flags: checkin+
(Assignee)

Updated

5 years ago
status-firefox28: affected → fixed
(Assignee)

Comment 13

5 years ago
Transplanted to Aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/1e94648caf90 (mozilla-aurora)

Reports
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1d7ff6

Remote: http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a1d999c
status-firefox27: affected → fixed
status-firefox29: affected → ---
(Assignee)

Comment 14

5 years ago
Created attachment 8339292 [details] [diff] [review]
beta.patch

Patch for the mozilla-beta and mozilla-release branches.

Beta testruns
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1e4def
Remote: http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a1ed50f
Attachment #8339292 - Flags: review?(hskupin)
Attachment #8339292 - Flags: review?(andreea.matei)
Comment on attachment 8339292 [details] [diff] [review]
beta.patch

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/6945d8325443 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/aeee099df7f8 (release)
Attachment #8339292 - Flags: review?(hskupin)
Attachment #8339292 - Flags: review?(andreea.matei)
Attachment #8339292 - Flags: review+
Created attachment 8339309 [details] [diff] [review]
[esr24] patch v1

Backported to esr24.
Attachment #8339309 - Flags: review?(hskupin)
Created attachment 8339344 [details] [diff] [review]
[esr17] patch v1

Here is the backport patch for esr17, I didn't had time to run full testruns for functional and remote, though I run the restarts tests locally and they are green. 

Thanks
Attachment #8339344 - Flags: review?(hskupin)
Attachment #8339344 - Flags: review?(andreea.matei)
(Reporter)

Updated

5 years ago
status-firefox25: affected → fixed
status-firefox26: affected → fixed
(Reporter)

Comment 18

5 years ago
Comment on attachment 8339309 [details] [diff] [review]
[esr24] patch v1

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

I have problems with the interdiff view but running the tests all pass. So I think we should be good here. Thank you Andreea.

Pushed to esr24:
http://hg.mozilla.org/qa/mozmill-tests/rev/e14f189aa94e
Attachment #8339309 - Flags: review?(hskupin)
Attachment #8339309 - Flags: review+
Attachment #8339309 - Flags: checkin+
(Reporter)

Comment 19

5 years ago
Comment on attachment 8339344 [details] [diff] [review]
[esr17] patch v1

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

All passed.  So beside the nits i give r+ and get it landed.

http://hg.mozilla.org/qa/mozmill-tests/rev/a485de4f3491 (esr17)

::: tests/functional/restartTests/testAddons_enableDisableExtension/test4.js
@@ +26,5 @@
>    addons.resetDiscoveryPaneURL();
>    addonsManager.close();
>  
>    prefs.preferences.clearUserPref(PREF_UPDATE_EXTENSION);
> +  prefs.preferences.clearUserPref(PREF_LAST_CATEGORY);

Usually this should be in alphabetical order. But given that esr17 will be dead soon, I'm ok with it.

::: tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +10,5 @@
>  var tabs = require("../../../lib/tabs");
>  
>  const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
>  const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html';
> +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";

Again, those constants are of different functional area and should be in a separated block. Please read the coding style guide. This time I'm ok with it on a nearly dead branch.
Attachment #8339344 - Flags: review?(hskupin)
Attachment #8339344 - Flags: review?(andreea.matei)
Attachment #8339344 - Flags: review+
(Reporter)

Comment 20

5 years ago
All done here. Closing as fixed. Thanks everyone.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr17: affected → fixed
status-firefox-esr24: affected → fixed
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
No longer blocks: 939690
(Reporter)

Updated

5 years ago
Blocks: 744007
You need to log in before you can comment on or make changes to this bug.