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

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

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

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?

Comment 1

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

To reproduce that just run 'mozmill --profile test -b %path% -m %manifest_to_tests'.

Comment 2

[Andrei Eftimie]

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

Comment 3

[Andrei Eftimie]

Attachment: 1.patch

I think I got all of them.

I'll be posting some testrun results shortly

Comment 4

[Andrei Eftimie]

Attachment: 1.patch

Missed the last change I did from the previous patch

Comment 5

[Andrei Eftimie]

Attachment: 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

Comment 6

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

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.

Comment 7

[Andrei Eftimie]

Attachment: 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

Comment 8

[Andrei Eftimie]

Functional testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a0296e4

Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a02a074

Comment 9

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

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?

Comment 10

[Andrei Eftimie]

Attachment: 4.patch

Updated the pref for installFromFTP

Same remote result: http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a03ca67

Comment 11

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

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

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

Looks fine. Please get it landed.

Comment 12

[Andrei Eftimie]

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)

Comment 13

[Andrei Eftimie]

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

Comment 14

[Andrei Eftimie]

Attachment: 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

Comment 15

[Andreea Matei [:AndreeaMatei]]

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)

Comment 16

[Andreea Matei [:AndreeaMatei]]

Attachment: [esr24] patch v1

Backported to esr24.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: [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

Comment 18

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

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

Comment 19

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

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.

Comment 20

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

All done here. Closing as fixed. Thanks everyone.