Closed Bug 987612 Opened 7 years ago Closed 6 years ago

The restart functional tests for 'Blocklist Extension' shouldn't fail with no network connection

Categories

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

defect

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr24 wontfix, firefox-esr31 fixed)

RESOLVED FIXED
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr24 --- wontfix
firefox-esr31 --- fixed

People

(Reporter: danisielm, Assigned: andrei)

References

(Depends on 1 open bug)

Details

(Whiteboard: [sprint])

Attachments

(3 files, 12 obsolete files)

32.97 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
34.02 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
With no connection to the internet, this functional tests will fail when trying to update the blocklist.

> firefox/tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension
> firefox/tests/functional/restartTests/testAddons_installUninstallSoftBlocklistedExtension

An approach here will be to use our local data/addons/blocklist/(hard/soft)block_extension/blocklist.xml from where to update it. This, I guess, can be done via extensions.blocklist.url pref, but I'm not sure.
Exactly this we are doing at the moment. Not sure why you think that we use a remote url here. Any reason for that, Daniel?
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Exactly this we are doing at the moment. Not sure why you think that we use
> a remote url here. Any reason for that, Daniel?

You're right, sorry for missing that.
This tests still fails with no network connection because we call the 'updateBlocklist' in the last test's 'teardownModule' _after_ we clear the pref, that's the moment we use remote connection to ping the AMO.

Henrik, do we really need that update the blocklist in the teardownModule ?
Flags: needinfo?(hskupin)
Version: Version 1 → unspecified
Ups. Good catch! So do we have a reset blocklist method in the API, which we could call?
Flags: needinfo?(hskupin)
Blocks: 950025
The reset blocklist means reseting the blocklist.url pref which we already do.

I currently can't find a method in the API that resets/sets the actually local saved blocklist according to some local data and not the one from the default AMO url, given that that's the default behavior.

I believe the blocklist is saved to:
http://mxr.mozilla.org/mozilla-central/source/browser/app/blocklist.xml.

I suggest we remove that update blocklist call from tearDownModule, or improve it so we don't fail if the service doesn't respond.
The blocklist is saved inside the profile. The file is named accordingly. So we might want to force a reset profile after such a test. To be safe both tests would have to be changed to resist in a single file.
We had a failure today with "Blocklist has been updated." on line : http://hg.mozilla.org/qa/mozmill-tests/file/ba4b105952bc/firefox/lib/utils.js#l528 .

This hapened in the teardown module of the restartTests\testAddons_installUninstallHardBlocklistedExtension\test4.js test when updateing the blocklist with the one from amo.
Looks like the best option we have here to get it fixed is my comment 5.
We-ve seen a lot of failures lately with this.
I'll take & fix it.
Priority: -- → P2
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Attached patch v1.patch (obsolete) — Splinter Review
Attachment #8472325 - Flags: review?(andrei.eftimie)
Attachment #8472325 - Flags: review?(andreea.matei)
Comment on attachment 8472325 [details] [diff] [review]
v1.patch

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

Great work Daniel!
These now work nicely without an internet connection.

Just a couple small changes from my part. Please do them in both tests.

::: firefox/tests/functional/testAddons/testInstallUninstallHardBlocklistedExtension.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Include required modules
> +var { expect } = require("../../../../lib/assertions");

This can be safely removed.

@@ +13,5 @@
> +var tabs = require("../../../lib/tabs");
> +var utils = require("../../../../lib/utils");
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_DATA = BASE_URL + "addons/blocklist/hardblock_extension/blocklist.xml";

Please make only one TEST_DATA object.
It should contain both TEST_DATA.blocklist and TEST_DATA.addon (from below)

@@ +73,5 @@
> + */
> +function testInstallBlocklistedExtension() {
> +  persisted.nextTest = "testBlocklistsExtension";
> +
> +  addons.setDiscoveryPaneURL("about:home");

This should live in setupModule
Attachment #8472325 - Flags: review?(andrei.eftimie)
Attachment #8472325 - Flags: review?(andreea.matei)
Attachment #8472325 - Flags: review-
Since Daniel is on PTO from tomorrow, Cosmin please take over and update this patch tomorrow.
Assignee: daniel.gherasim → cosmin.malutan
Attached patch patch v1.1 (obsolete) — Splinter Review
Hi Andrei, I fixed what you requested and I made a ran to be sure it works.
Best
Attachment #8472325 - Attachment is obsolete: true
Attachment #8472857 - Flags: review?(andrei.eftimie)
Comment on attachment 8472857 [details] [diff] [review]
patch v1.1

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

There's a missing comma which invalidates the test.
Otherwise this looks and performs well.

With that updates ask a review from Henrik. This looks great to me.

::: firefox/tests/functional/testAddons/testInstallUninstallHardBlocklistedExtension.js
@@ +13,5 @@
> +var utils = require("../../../../lib/utils");
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_DATA = {
> +  blocklist: BASE_URL + "addons/blocklist/hardblock_extension/blocklist.xml"

You forgot a comma here.
Attachment #8472857 - Flags: review?(andrei.eftimie) → review-
Attached patch patch v1.2 (obsolete) — Splinter Review
I missed to refresh before exporting, fixed.
Attachment #8472857 - Attachment is obsolete: true
Attachment #8472912 - Flags: review?(hskupin)
Blocks: 1057406
Comment on attachment 8472912 [details] [diff] [review]
patch v1.2

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

That's what I would like to see for all of our restart tests! It's great. So maybe we should file bugs for all existent tests, which need to be transformed.

There is only one nit you will have to fix. Otherwise it looks like what I proposed in comment 5. So lets get this in, and the failure killed. Thanks!

::: firefox/tests/functional/testAddons/testInstallUninstallHardBlocklistedExtension.js
@@ +74,5 @@
> + */
> +function testInstallBlocklistedExtension() {
> +  persisted.nextTest = "testBlocklistsExtension";
> +
> +

nit: Kill this extra empty line.
Attachment #8472912 - Flags: review?(hskupin) → review+
Attached patch patch v1.3 (obsolete) — Splinter Review
Fixed the nit.
Attachment #8472912 - Attachment is obsolete: true
Attachment #8486326 - Flags: checkin?(andrei.eftimie)
Comment on attachment 8486326 [details] [diff] [review]
patch v1.3

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/aeb8e247acf5 (default)
Attachment #8486326 - Flags: review+
Attachment #8486326 - Flags: checkin?(andrei.eftimie)
Attachment #8486326 - Flags: checkin+
I can reproduce the issue with running the hardblocklisted test before the softblocklisted one with the same profile.

Not exactly sure yet what is happening, stopApplication(true) might not clean everything we expect up? I remember we've stumbled across this before.
So what exactly fails here? I can see that we do not handle the modal dialog for installing the add-on. Is the old blocklist file still part of the profile?
%profile%/blocklist.xml doesn't get cleaned (removed) between these tests, since we're using the same addons for them the first test invalidates the next.

We should check why this is happening. Best solution would be to make sure the blocklist file is properly cleaned. Will check if mozprofile should do this.
Sounds clearly like a mozprofile bug. In case of a reset, really all the files should be removed.
We are actually using different extensions (1 for each test), yet the blocklist.xml file contains both for each blocklisted test. I'm testing this hypothesis now.

I've also looked at mozprofile and the cleanup method basically does 3 things:
> self.addon_manager.clean()
> self.permissions.clean_db()
> self.webapps.clean()

None of which appear to clean the blocklist
Attached patch fix2.patch (obsolete) — Splinter Review
I've cleaned up the local blocklist.xml files to contain only the used addon for each of the tests. This fixes the issue we're having. 

http://mozmill-crowd.blargon7.com/#/functional/report/b509c8df3c5f32f8b1b39e28be0cca18

I'll shortly file a bug against mozprofile since addon.clean() should also take care of the blocklist besides removing the addons.
Assignee: cosmin.malutan → andrei.eftimie
Attachment #8486326 - Attachment is obsolete: true
Attachment #8486454 - Flags: review?(andreea.matei)
Filed bug 1064916 against mozprofile
I don't think we should be "hard" blocked by the mozprofile issue. With the current patch everything works nicely (as we avoid the issue from bug 1064916)
Comment on attachment 8486454 [details] [diff] [review]
fix2.patch

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

With the latest patch you are regressing the blocklist regarding severity settings for blocklisted add-ons. We shouldn't change it that way just to workaround a problem. Better would be to create a deleteFile() method for the time being in the files.js module, and remove the blocklist file on our own in teardownModule.
Attachment #8486454 - Flags: review?(andreea.matei) → review-
(In reply to Henrik Skupin (:whimboo) from comment #28)
> With the latest patch you are regressing the blocklist regarding severity
> settings for blocklisted add-ons. We shouldn't change it that way just to
> workaround a problem.

Severity ratings:
> Severity ratings:
> 0 = out of date
> 1 = soft blocked
> 2 = hard blocked
> 3 = default

In `testInstallUninstallHardBlocklistedExtension.js` we use `data/addons/blocklist/hardblock_extension/blocklist.xml` and we only test the addon `test-icons@quality.mozilla.org`. This addon has no specific severity, so it defaults to 3.

This `blocklist.xml` file is not used anywhere else. Thus the other 2 addons mentioned in this file are not used anywhere else. I don't see what we regress in removing not-used code.

The same applies to the softblocklisted test.

If we would use them anywhere sure, but I can't see what we regress here?

> Better would be to create a deleteFile() method for
> the time being in the files.js module, and remove the blocklist file on our
> own in teardownModule.
This is just another workaround, in what ways is this better? As you said before, this should be addressed in mozprofile.
We added those add-ons to the blocklist to be able to test all three different severity states. We simply haven't created the tests yet. Removal of those entries will cause us to have them added back again.

The deletion of the file in our test I do not call a workaround. Having such a method in files.js would always be preferred. I think might be useful also for other stuff. Then having a simple call should be fine to get it removed. Addressing this in mozprofile is a way larger thing to do, which will not happen in the short-time. Mainly because its not clear how existing profiles should be handled in regards of cleaning it up.
Attached patch fix3.patch (obsolete) — Splinter Review
I've reverted the changes to the `blocklist.xml` files.
I've added a new method to remove files, and some helper methods to get resources from the profile folder.

All tests are running fine.
Attachment #8486454 - Attachment is obsolete: true
Attachment #8487083 - Flags: review?(andreea.matei)
Comment on attachment 8487083 [details] [diff] [review]
fix3.patch

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

Looks good, tests well. But doesn't apply anymore on utils.js and what I'd like is to have less blank lines in the tests, I see variables and assert calls that are separated by a blank line. Also where there's a comment which is pretty obvious already from the message of the assert, that can be dropped and so remove the blank line as well (cause I know we use one before comments). The tests are now long enough, if we could reduce them a bit and still keep readability, would be great.
Attachment #8487083 - Flags: review?(andreea.matei) → review-
Attached patch fix4.patch (obsolete) — Splinter Review
Fixed conflicts.
Removed some empty newlines from the tests.
Attachment #8487083 - Attachment is obsolete: true
Attachment #8491277 - Flags: review?(andreea.matei)
Comment on attachment 8491277 [details] [diff] [review]
fix4.patch

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

Thanks, moving review to Henrik.
Attachment #8491277 - Flags: review?(hskupin)
Attachment #8491277 - Flags: review?(andreea.matei)
Attachment #8491277 - Flags: review+
Comment on attachment 8491277 [details] [diff] [review]
fix4.patch

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

::: firefox/tests/functional/testAddons/testInstallUninstallSoftBlocklistedExstension.js
@@ +29,5 @@
> +
> +const INSTALL_DIALOG_DELAY = 1000;
> +const TIMEOUT_DOWNLOAD = 25000;
> +
> +const PROFILE_BLOCKLIST = utils.getProfileResource('blocklist.xml');

Lets use PATH_ as prefix for such instances, e.g. PATH_BLOCKLIST_FILE. PROFILE is not really related here, given that the blocklist might also be located somewhere else.

::: lib/files.js
@@ +42,5 @@
>    return data;
>  }
>  
>  /**
> + * Removes a file

nit: 'Remove a file'

@@ +45,5 @@
>  /**
> + * Removes a file
> + *
> + * All good if the file does not exist.
> + * Will throw for anything else that goes wrong.

There is a @throws statement. Please use that instead of those two lines.

@@ +52,5 @@
> + *        File to be deleted
> + * @param {boolean} [aRecursive=false]
> + *        Need to be true to recursively remove folders
> + */
> +function removeFile(aFile, aRecursive = false) {

As said on another bug already, there is no space between the parameter, operand, and value.

@@ +59,5 @@
> +  } catch (e) {
> +    switch (e.name) {
> +      case "NS_ERROR_FILE_TARGET_DOES_NOT_EXIST":
> +        // We're trying to remove the file, if it doesn't exist, success!
> +        break;

You can use 'catch (e if e.name == "...EXIST")' to handle that specific exception. All others should not be caught, so no need for the second half of the switch and the rethrow statement.

::: lib/utils.js
@@ +353,3 @@
>   */
> +function getProfileLocation() {
> +  return Services.dirsvc.get("ProfD", Ci.nsIFile);

I would drop that method in favor of an optional parameter to getProfileResource().

@@ +369,1 @@
>  }

I do not see why this got added to utils. All that is file system related and should be added to the newly created files module.
Attachment #8491277 - Flags: review?(hskupin) → review-
Attached patch fix5.patch (obsolete) — Splinter Review
- made the variable name and the comments changes.
- simplified the `catch` from removeFile.
- removed the `getProfileLocation` method, and made `getProfileResource` return the profile location when called without arguments
- added a test for both these changes
- moved them from `utils` into `files`
Attachment #8491277 - Attachment is obsolete: true
Attachment #8493718 - Flags: review?(andreea.matei)
Comment on attachment 8493718 [details] [diff] [review]
fix5.patch

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

I see everything mentioned in the last review was implemented, tested it and works fine. Leaving a final look for Henrik.
Attachment #8493718 - Flags: review?(hskupin)
Attachment #8493718 - Flags: review?(andreea.matei)
Attachment #8493718 - Flags: review+
Comment on attachment 8493718 [details] [diff] [review]
fix5.patch

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

r- mainly because it doesn't apply anymore. Lets get the remaining bits fixed, and it should be ready to get landed.

::: firefox/tests/functional/testAddons/testInstallUninstallHardBlocklistedExtension.js
@@ +68,5 @@
> +
> +  delete persisted.addon;
> +  delete persisted.nextTest;
> +
> +  files.removeFile(PATH_BLOCKLIST_FILE);

This will need an update given that this function is encapsulated in the File class now. On the other side we could add a new global method as a wrapper to simply the usage.

::: lib/files.js
@@ +15,5 @@
>  const iniFactory = Cc["@mozilla.org/xpcom/ini-processor-factory;1"]
>                     .getService(Ci.nsIINIParserFactory);
>  
> +/**
> + * Get a resource from a profile

nit: 'from the profile location'. Maybe we could even let the user specify the location by letting the 'ProfD' constant be a parameter, and make the method more general. That way we can use it even for resources under the apps folder. You can file this as a follow-up bug.

@@ +19,5 @@
> + * Get a resource from a profile
> + *
> + * @param {string} [aPath=""]
> + *        Path for the resource
> + * @return {nsIFile} requested file object

nit: Capital letter for `requested` please

@@ +21,5 @@
> + * @param {string} [aPath=""]
> + *        Path for the resource
> + * @return {nsIFile} requested file object
> + */
> +function getProfileResource(aPath="") {

I would suggest that we file another bug, where we can handle an unkown number of parameters here, so we can construct a URL even for e.g. profile/extensions/xyz.xpi.

@@ +148,5 @@
>  /**
> + * Remove a file
> + *
> + * @param {nsiFile} aFile
> + *        File to be deleted

I would say we allow a simple path here. Otherwise the new File class should be used. But yes, lets add such a metho to the File class too.

@@ +156,5 @@
> + */
> +function removeFile(aFile, aRecursive=false) {
> +  try {
> +    aFile.remove(aRecursive);
> +  } catch (e if e.name === "NS_ERROR_FILE_TARGET_DOES_NOT_EXIST") {}

nit: ending bracket to the new line.

::: lib/tests/manifest.ini
@@ +5,5 @@
>  [testFormData.js]
>  [testOpenMultipleWindows.js]
>  [testPerformance.js]
>  [test_assertions.js]
> +[test_files.js]

Oh, looks like I missed the tests for my recent updates for this module. :/ Once this patch is in we should file a bug to get the missing tests added.

::: lib/tests/test_files.js
@@ +18,5 @@
> +  var file = files.getProfileResource("nonexistant.file");
> +
> +  // Trying to remove a nonexitant file should not throw
> +  files.removeFile(file);
> +}

With the follow-up patch we should also add a test for the real deletion of a file.
Attachment #8493718 - Flags: review?(hskupin) → review-
Attached patch fix6.patch (obsolete) — Splinter Review
- `remove` is now a method of the File class
- updated nits
- added support for deep linking by passing in an array of path leafs to `getProfileResource`
- added tests for the recently added File class and for removing a file
- for custom locations "greD" we'll implement this later when needed

Report: http://mozmill-crowd.blargon7.com/#/functional/report/f11964362bbc85ebfbc5b36de7da5929
Attachment #8493718 - Attachment is obsolete: true
Attachment #8496925 - Flags: review?(andreea.matei)
This fails for me with a disconnect error, everytime I've ran the manifest in testAddons with Nightly. Please check if you can reproduce this as well.
If you can reproduce this disconnect all the time, I don't see a value asking someone else to reproduce. It would be better if you would investigate the underlying issue, means why this disconnect is happening. This will safe the time...
Comment on attachment 8496925 [details] [diff] [review]
fix6.patch

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

I think was something on my machine, I've just switched to it that day, it doesn't reproduce anymore now. 
Passing to final look.
Attachment #8496925 - Flags: review?(hskupin)
Attachment #8496925 - Flags: review?(andreea.matei)
Attachment #8496925 - Flags: review+
Comment on attachment 8496925 [details] [diff] [review]
fix6.patch

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

::: firefox/tests/functional/testAddons/testInstallUninstallHardBlocklistedExtension.js
@@ +28,5 @@
> +
> +const INSTALL_DIALOG_DELAY = 1000;
> +const TIMEOUT_DOWNLOAD = 25000;
> +
> +const BLOCKLIST_FILE = new files.File(files.getProfileResource('blocklist.xml'));

Personally I think it would be good when we keep the top-level constants to simple declarations. Means without instantiating new classes. So please move this File instantiation to the teardownModule method, and keep the code you had before, which was fine.

@@ +34,5 @@
> +function setupModule(aModule) {
> +  persisted.addon = TEST_DATA.addon;
> +
> +  addons.setDiscoveryPaneURL("about:home");
> +  prefs.preferences.setPref(PREF_BLOCKLIST, TEST_DATA.blocklist);

We are updating the pref but never actually trigger a blocklist update via utils.updateBlocklist()! This could cause failures as we have seen in the past for those tests.

@@ +37,5 @@
> +  addons.setDiscoveryPaneURL("about:home");
> +  prefs.preferences.setPref(PREF_BLOCKLIST, TEST_DATA.blocklist);
> +  prefs.preferences.setPref(PREF_INSTALL_DIALOG, INSTALL_DIALOG_DELAY);
> +
> +  BLOCKLIST_FILE.remove();

Do we really need this call here? The blocklist gets overwritten anyway. IMO we can save some time here.
Attachment #8496925 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #43)
> @@ +34,5 @@
> > +function setupModule(aModule) {
> > +  persisted.addon = TEST_DATA.addon;
> > +
> > +  addons.setDiscoveryPaneURL("about:home");
> > +  prefs.preferences.setPref(PREF_BLOCKLIST, TEST_DATA.blocklist);
> 
> We are updating the pref but never actually trigger a blocklist update via
> utils.updateBlocklist()! This could cause failures as we have seen in the
> past for those tests.

We do when we open the blocklistWindow:
http://hg.mozilla.org/qa/mozmill-tests/file/0a0d59ce762a/firefox/lib/ui/addons_blocklist.js#l30

> @@ +37,5 @@
> > +  addons.setDiscoveryPaneURL("about:home");
> > +  prefs.preferences.setPref(PREF_BLOCKLIST, TEST_DATA.blocklist);
> > +  prefs.preferences.setPref(PREF_INSTALL_DIALOG, INSTALL_DIALOG_DELAY);
> > +
> > +  BLOCKLIST_FILE.remove();
> 
> Do we really need this call here? The blocklist gets overwritten anyway. IMO
> we can save some time here.

We need to make sure we're clean before going forward, otherwise the first test will fail trying to install the addon (if it's blocklisted).
I think the current name of the folder is misleading what's actually been tested. So yes, we install the add-on before updating the blocklist. But I do not think that updating the blocklist in .open() is correct. That method should really be called inside the test at that point where the blocklist has to be modified. In most cases when the blocklist window should open, the blocklist should have already been updated. Keeping this in mind I currently cannot see where we update the blocklist so that the blocklist windows is getting opened. Do I miss something?
Attached patch fix7.patch (obsolete) — Splinter Review
I've been wrapping my head around this, and I don't think we can easily solve this issue without a major rewrite of the addons_blocklist library.

This is because the following:
- the blocklist window is opened by the blocklist notifier call in utils.js
- the "blocklist-update" event is fired _after_ the blocklist window has been handled

Thus we will have to break the updateBlocklist method apart, since this method always opens the UI window, I'd say to move it into the blocklist class. I imagine we'll want to open the blocklist window with a callback on what to do while the window is opened, and the we'll wait for "blocklist-update" when we close the window.

===

The simple solution right now (which does work) is to keep doing what we were before, in addons_blocklist we:
- we call utils.updateBlocklist which opens the blocklistwindow
- we save the controller for the opened window
- in test we handle it
> open : function BlocklistWindow_open() {
>   utils.updateBlocklist();
> 
>   this._controller = windows.handleWindow("type", "Addons:Blocklist", undefined, false);
> },

Since the "wait" argument in "updateBlocklist" has always been broken (this can't work with the current implementation) I'd remove it completely, and add it when we refactor this as said above.
Attachment #8496925 - Attachment is obsolete: true
Attachment #8506893 - Flags: review?(andreea.matei)
Attachment #8506893 - Flags: feedback?(hskupin)
Comment on attachment 8506893 [details] [diff] [review]
fix7.patch

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

(In reply to Andrei Eftimie from comment #46)
> I've been wrapping my head around this, and I don't think we can easily
> solve this issue without a major rewrite of the addons_blocklist library.

I don't think it will such a big rewrite because:

> - the blocklist window is opened by the blocklist notifier call in utils.js

We might have to adapt the window code to be based on BaseWindow. With that we get its opening method, where we can pass in the updateBlocklist() call wrapped into a callback. So this would be most safe.

> - the "blocklist-update" event is fired _after_ the blocklist window has
> been handled

Not sure I understand. When we want to bring up the blocklist window, we have to update the blocklist before. Otherwise the window doesn't appear for certain operations.

> Thus we will have to break the updateBlocklist method apart, since this
> method always opens the UI window, I'd say to move it into the blocklist
> class. I imagine we'll want to open the blocklist window with a callback on
> what to do while the window is opened, and the we'll wait for
> "blocklist-update" when we close the window.

The window only opens immediately if you have versions of addons installed, which are listed in the blocklist, or when you are going to install such an equivalent addon once the new blocklist is in place. The window will not appear otherwise! So this is still a backend method to get the blocklist updated, and should not be moved into a class of a ui module.

> Since the "wait" argument in "updateBlocklist" has always been broken (this
> can't work with the current implementation) I'd remove it completely, and
> add it when we refactor this as said above.

Can you please tell me what's wrong with it? I don't see your point here even checking the method. We only continue in this case when the blocklist-update observer notification is fired.

::: firefox/lib/ui/addons_blocklist.js
@@ +27,5 @@
>    /**
>     * Open the Blocklist Window
>     */
>    open : function BlocklistWindow_open() {
> +    utils.updateBlocklist();

This line we should really get out of this method. That's not how the open method should work. Lets make BlocklistWindow to subclass BaseWindow and handle opening the window correctly similar to the other new ui modules and associated tests.

Also not sure why we have specified false here as parameter. It hides the information if the observer notification has not been fired. So we only noticed that because the window didn't open.
Attachment #8506893 - Flags: review?(andreea.matei)
Attachment #8506893 - Flags: feedback?(hskupin)
Attachment #8506893 - Flags: feedback-
Let me try rephrasing.

The only action that opens the blocklist window is: utils.updateBlocklist();
This is the mechanism we are using (and the only one I am aware of ATM).

The "blocklist-updated" event is fired _after_ the blocklist window has been closed.

Thus the code we have now when calling utils.updateBlocklist(true); is broken.
It will always timeout because we are waiting for the event without handling (and closing) the blocklist window.
(In reply to Andrei Eftimie from comment #48)
> The only action that opens the blocklist window is: utils.updateBlocklist();
> This is the mechanism we are using (and the only one I am aware of ATM).

There are a variety of other actions which could open it. Lets say you call updateBlocklist() but you don't have any blocklisted addon installed. In this case no window will be opened! In our current implementation we will fail because we expect a window to be opened. So if you try to install such an addon later with the same updated blocklist in place, you should get this window after clicking the install button.

> The "blocklist-updated" event is fired _after_ the blocklist window has been
> closed.

Where do you get this from? Keep in mind that we weren't waiting for this event to happen yet! So it could also mean that our test is so fast and closed the window again. There might still other things happen in the background before the observer is fired, which not only involves opening the blocklist window.

> Thus the code we have now when calling utils.updateBlocklist(true); is
> broken.
> It will always timeout because we are waiting for the event without handling
> (and closing) the blocklist window.

So I did a search on MXR and there are indeed several paths when this observer is fired. It really depends on if blocklisted elements exists or not:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#1205

This code needs to be analyzed and the different cases layed out. That would help us a lot in determining the correct logic for us, e.g. only use true for the wait flag when we do not expect the window, but false if we do.
Blocks: 1092145
Attached patch fix8.patch (obsolete) — Splinter Review
As discussed in last weeks Ask an Expert meeting, here's an update based on previous v6 patch.

Also filed bug 1092145 to enhance the blocklist classes with browser window, and investigate how to treat update blocklist in regards to opening of the blocklist window properly.
Attachment #8506893 - Attachment is obsolete: true
Attachment #8514973 - Flags: review?(andreea.matei)
Attached patch fix9.patch (obsolete) — Splinter Review
Another update. Seems I didn't upload quite the latest patch version + some bitrot.

Now I also updated addons.js setCategory to return early if the requested category is already selected.

If a category remains selected (about:addons seems to keep its state) and in a subsequent test you attempt to select the same category the test would fail with "Category has been selected" because we are waiting of a change observer. This should fix multiple issues we might have here.

Also report: http://mozmill-crowd.blargon7.com/#/functional/report/1e5e44bea8f3e17708194a929a8c3cf0
Attachment #8514973 - Attachment is obsolete: true
Attachment #8514973 - Flags: review?(andreea.matei)
Attachment #8517474 - Flags: review?(andreea.matei)
Comment on attachment 8517474 [details] [diff] [review]
fix9.patch

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

This tests well and has the changes requested, where possible (otherwise explained in comment 44).
Attachment #8517474 - Flags: review?(hskupin)
Attachment #8517474 - Flags: review?(andreea.matei)
Attachment #8517474 - Flags: review+
Whiteboard: [sprint]
Comment on attachment 8517474 [details] [diff] [review]
fix9.patch

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

Thank you for the update Andrei. There are only a couple of nits remaining. Once fixed you have my r=me.

::: firefox/tests/functional/testAddons/testInstallUninstallHardBlocklistedExtension.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  // Include required modules
> +var { BlocklistWindow } = require("../../../lib/ui/addons_blocklist");

nit: Please split ui from backend modules.

@@ +95,5 @@
> + */
> +function testBlocklistsExtension() {
> +  persisted.nextTest = "testUninstallBlocklistedExtension";
> +
> +  var blocklistWindow = new BlocklistWindow(controller);

nit: Please move it down to when you actually work with this instance.

::: firefox/tests/functional/testAddons/testInstallUninstallSoftBlocklistedExtension.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  // Include required modules
> +var { BlocklistWindow } = require("../../../lib/ui/addons_blocklist");

Same comments as for the hardblocked test apply here.

::: lib/files.js
@@ +77,5 @@
>      foStream.close();
> +  },
> +
> +
> +  /**

nit: remove one of the blank lines.

@@ +194,5 @@
>  
> +/**
> + * Get a resource from the profile location
> + *
> + * @param {string|string[]} [aPath=[""]

There is a missing closing ']' bracket. Maybe we should default to a string for a better visibility? It doesn't matter if we create the array in the function head or inside its body.
Attachment #8517474 - Flags: review?(hskupin) → review+
Attached patch fix10.patchSplinter Review
Thanks Andreea and Henrik for the reviews. Carrying over the r+
Fixed all nits.

Rerun tests to make sure nothing regressed and both Blocklist tests are passing without a working internet connection.

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/95030cc711bc (default)
Attachment #8517474 - Attachment is obsolete: true
Attachment #8521333 - Flags: review+
Attachment #8521333 - Flags: checkin+
(In reply to Andrei Eftimie from comment #54)
> Rerun tests to make sure nothing regressed and both Blocklist tests are
> passing without a working internet connection.

Fantastic! Great work Andrei. Now we only have to refactor the window ui class to handle it correctly.
Transplanted to Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/79a738a8e71e (mozilla-aurora)

All is green as well, transplant had 1 minor conflict in the manifest file.
Going to let this bake on 36 and 35 before backporting further.
Flags: needinfo?(andrei.eftimie)
Blocks: 967456
Depends on: 1097700
When I run the tests in Jenkins I see something interesting:

00:03:32.399 Included file 'c:\jenkins\workspace\mozilla-central_functional\data\mozmill-tests\firefox\tests\functional\restartTests\testAddons_installUninstallHardBlocklistedExtension\manifest.ini' does not exist
00:03:32.399 Included file 'c:\jenkins\workspace\mozilla-central_functional\data\mozmill-tests\firefox\tests\functional\restartTests\testAddons_installUninstallSoftBlocklistedExtension\manifest.ini' does not exist

This is before the tests start to run.
Thanks Henrik for the catch.

I clearly remember having this at one point, and fixing it. Somehow I messed up and it didn't land with the final patch.

Patch for clearing remaining missing manifest inclusions.
Attachment #8521999 - Flags: review?(andreea.matei)
Comment on attachment 8521999 [details] [diff] [review]
manifest_fix.patch

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

Looks good. Please land it.
Attachment #8521999 - Flags: review?(andreea.matei) → review+
https://hg.mozilla.org/qa/mozmill-tests/rev/b8934759b1f6 (default)
https://hg.mozilla.org/qa/mozmill-tests/rev/399ffa453f91 (mozilla-aurora)

I'll prepare a combined patch for Beta with this fix, and bug 1097700
Attached patch fix_beta.patchSplinter Review
This patch is for beta. It has both fixes folded in.

We have the last beta build tomorrow. I'd like to land this after. But before the merge on Monday evening.
Flags: needinfo?(andrei.eftimie)
Attachment #8522120 - Flags: review?(andreea.matei)
(In reply to Andrei Eftimie from comment #63)
> Transplanted to beta:
> https://hg.mozilla.org/qa/mozmill-tests/rev/3743ab4c2df3 (mozilla-beta)

Argh, I shouldn't have pushed this, I had another patch with 2 fixes folded in.

Backed out:
https://hg.mozilla.org/qa/mozmill-tests/rev/b2c0c8fc3a36 (mozilla-beta)

Andreea please have a look at the patch from comment 61
Comment on attachment 8522120 [details] [diff] [review]
fix_beta.patch

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

Tested well for me and seems to have all fixes.
Attachment #8522120 - Flags: review?(andreea.matei) → review+
Landed in Beta:
https://hg.mozilla.org/qa/mozmill-tests/rev/8360f326898b (mozilla-beta)
Transplanted to ESR31 (fixed a couple minor conflicts):
https://hg.mozilla.org/qa/mozmill-tests/rev/e713b6bb3ec2 (mozilla-esr31)

All green:
http://mozmill-crowd.blargon7.com/#/remote/report/635fcffb06b246be47416301bd49f58e
http://mozmill-crowd.blargon7.com/#/functional/report/635fcffb06b246be47416301bd49e9c9

No need to land this on 33 as 34 will get merged into it on a couple of days.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Andrei Eftimie from comment #67)
> No need to land this on 33 as 34 will get merged into it on a couple of days.

I would still prefer to get this landed, given that it makes the merge patch smaller and less error-prone. But it's up to you here. I'm more for consistency than spending an extra minute for its landing.
(In reply to Henrik Skupin (:whimboo) from comment #68)
> (In reply to Andrei Eftimie from comment #67)
> > No need to land this on 33 as 34 will get merged into it on a couple of days.
> 
> I would still prefer to get this landed, given that it makes the merge patch
> smaller and less error-prone. But it's up to you here. I'm more for
> consistency than spending an extra minute for its landing.

We shouldn't worry about merge size or conflicts since we do a diff and merge everything anyway.
Since we get everything in, there's shouldn't be any more errors.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.