Closed
Bug 987612
Opened 10 years ago
Closed 10 years ago
The restart functional tests for 'Blocklist Extension' shouldn't fail with no network connection
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(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.
Comment 1•10 years ago
|
||
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?
Reporter | ||
Comment 2•10 years ago
|
||
(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 ?
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(hskupin)
Version: Version 1 → unspecified
Comment 3•10 years ago
|
||
Ups. Good catch! So do we have a reset blocklist method in the API, which we could call?
Flags: needinfo?(hskupin)
Reporter | ||
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
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.
Reporter | ||
Comment 6•10 years ago
|
||
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.
Reporter | ||
Comment 7•10 years ago
|
||
> firefox/tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension http://mozmill-daily.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-02-03&test=%2FrestartTests%2FtestAddons_installUninstallHardBlocklistedExtension%2Ftest4.js&func=teardownModule > firefox/tests/functional/restartTests/testAddons_installUninstallSoftBlocklistedExtension http://mozmill-daily.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-02-03&test=%2FrestartTests%2FtestAddons_installUninstallSoftBlocklistedExtension%2Ftest4.js&func=teardownModule
Reporter | ||
Comment 9•10 years ago
|
||
We-ve seen a lot of failures lately with this. I'll take & fix it.
Priority: -- → P2
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Reporter | ||
Comment 10•10 years ago
|
||
Attachment #8472325 -
Flags: review?(andrei.eftimie)
Attachment #8472325 -
Flags: review?(andreea.matei)
Assignee | ||
Comment 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
Since Daniel is on PTO from tomorrow, Cosmin please take over and update this patch tomorrow.
Assignee: daniel.gherasim → cosmin.malutan
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox-esr24:
--- → wontfix
status-firefox-esr31:
--- → affected
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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-
Comment 15•10 years ago
|
||
I missed to refresh before exporting, fixed.
Attachment #8472857 -
Attachment is obsolete: true
Attachment #8472912 -
Flags: review?(hskupin)
Comment 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
Fixed the nit.
Attachment #8472912 -
Attachment is obsolete: true
Attachment #8486326 -
Flags: checkin?(andrei.eftimie)
Assignee | ||
Comment 18•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
status-firefox35:
--- → fixed
Assignee | ||
Comment 19•10 years ago
|
||
Backed out due to failures: https://hg.mozilla.org/qa/mozmill-tests/rev/e49e121e347a (default)
Assignee | ||
Comment 20•10 years ago
|
||
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.
Comment 21•10 years ago
|
||
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?
Assignee | ||
Comment 22•10 years ago
|
||
%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.
Comment 23•10 years ago
|
||
Sounds clearly like a mozprofile bug. In case of a reset, really all the files should be removed.
Assignee | ||
Comment 24•10 years ago
|
||
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
Assignee | ||
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
Filed bug 1064916 against mozprofile
Assignee | ||
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
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-
Assignee | ||
Comment 29•10 years ago
|
||
(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.
Comment 30•10 years ago
|
||
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.
Assignee | ||
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
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-
Assignee | ||
Comment 33•10 years ago
|
||
Fixed conflicts. Removed some empty newlines from the tests.
Attachment #8487083 -
Attachment is obsolete: true
Attachment #8491277 -
Flags: review?(andreea.matei)
Comment 34•10 years ago
|
||
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 35•10 years ago
|
||
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-
Assignee | ||
Comment 36•10 years ago
|
||
- 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 37•10 years ago
|
||
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 38•10 years ago
|
||
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-
Assignee | ||
Comment 39•10 years ago
|
||
- `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)
Comment 40•10 years ago
|
||
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.
Comment 41•10 years ago
|
||
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 42•10 years ago
|
||
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 43•10 years ago
|
||
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-
Assignee | ||
Comment 44•10 years ago
|
||
(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).
Comment 45•10 years ago
|
||
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?
Assignee | ||
Comment 46•10 years ago
|
||
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 47•10 years ago
|
||
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-
Assignee | ||
Comment 48•10 years ago
|
||
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.
Comment 49•10 years ago
|
||
(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.
Assignee | ||
Comment 50•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
status-firefox36:
--- → affected
Assignee | ||
Comment 51•10 years ago
|
||
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 52•10 years ago
|
||
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+
Updated•10 years ago
|
Whiteboard: [sprint]
Comment 53•10 years ago
|
||
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+
Assignee | ||
Comment 54•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Comment 55•10 years ago
|
||
(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.
Assignee | ||
Comment 56•10 years ago
|
||
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)
Comment 57•10 years ago
|
||
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.
Assignee | ||
Comment 58•10 years ago
|
||
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 59•10 years ago
|
||
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+
Assignee | ||
Comment 60•10 years ago
|
||
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
Assignee | ||
Comment 61•10 years ago
|
||
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)
Assignee | ||
Comment 62•10 years ago
|
||
Also reports on beta, all is green: http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c42eda9b http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c42ee000
Assignee | ||
Comment 63•10 years ago
|
||
Transplanted to beta: https://hg.mozilla.org/qa/mozmill-tests/rev/3743ab4c2df3 (mozilla-beta)
Assignee | ||
Comment 64•10 years ago
|
||
(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 65•10 years ago
|
||
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+
Assignee | ||
Comment 66•10 years ago
|
||
Landed in Beta: https://hg.mozilla.org/qa/mozmill-tests/rev/8360f326898b (mozilla-beta)
Assignee | ||
Comment 67•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Comment 68•10 years ago
|
||
(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.
Assignee | ||
Comment 69•10 years ago
|
||
(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.
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
•