Closed
Bug 1389488
Opened 7 years ago
Closed 7 years ago
addon.install should check if path of extension exists for a better error message
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Remote Protocol
Marionette
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: whimboo, Assigned: skeletor, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=js][lang=py])
User Story
Please make yourself familiar in how to run Marionette tests. Therefore check the steps as listed on the following wiki page: https://wiki.mozilla.org/User:Mjzffr/New_Contributors The file which needs to be modified can be found at: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/addon.js The API to use for the checks is: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile A test to check the behavior is here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_addons.py
Attachments
(1 file)
Right now addon.install() fails with a not that helpful error if the test is trying to install an add-on which does not exist: WebDriverException: Message: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: chrome://marionette/content/addon.js :: addon.install/< :: line 55" data: no] Instead lets check that the file exists before forwarding the request to the AddonManager.
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Comment 1•7 years ago
|
||
With my latest changes to the install addon code we have the following now: TEST-UNEXPECTED-ERROR | test_addons.py TestAddons.test_temporary_install_and_remove_unsigned_addon | AddonInstallException: Could not install add-on at '/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/tests/unit/ebextension-unsigned.xpi': Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.isFile] We could improve the failure message to something like: "Could not find add-on at ...".
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8911540 -
Flags: review?(hskupin)
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8911540 [details] Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager https://reviewboard.mozilla.org/r/182982/#review188322 Code-wise this looks fine. But please fix the indentation as mentioned inline. ::: testing/marionette/addon.js:94 (Diff revision 1) > */ > addon.install = async function(path, temporary = false) { > let file = new FileUtils.File(path); > let addon; > > + if(!file.exists()){ Please make sure to use white-spaces, otherwise the linter will fail. To check that as best run "./mach lint testing/marionette".
Attachment #8911540 -
Flags: review?(hskupin) → review+
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → rubberdonkeysandwich
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8911540 [details] Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager https://reviewboard.mozilla.org/r/182982/#review188332 Thanks. I triggered a try build so we can make sure nothing regresses. Something we missed, could you please also add a new Python test for this specific case?
Attachment #8911540 -
Flags: review+
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8911540 [details] Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager https://reviewboard.mozilla.org/r/182982/#review188698 ::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:95 (Diff revision 3) > addon_path = os.path.join(here, "webextension-unsigned.xpi") > > with self.assertRaises(AddonInstallException): > self.addons.install(addon_path) > + > + def test_install_nonexistent_addon_fails_with_helpful_message(self): Just name it `test_install_nonexistent_addon`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:98 (Diff revision 3) > self.addons.install(addon_path) > + > + def test_install_nonexistent_addon_fails_with_helpful_message(self): > + addon_path = os.path.join(here, "does-not-exist.xpi") > + > + with self.assertRaises(AddonInstallException) as cm: You should use https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaisesRegexp here, to directly compare the error message. It makes it much nicer to read. Also `cm` is not use here, so it can be removed.
Attachment #8911540 -
Flags: review?(hskupin) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8911540 [details] Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager https://reviewboard.mozilla.org/r/182982/#review188732 ::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:99 (Diff revisions 3 - 5) > > - def test_install_nonexistent_addon_fails_with_helpful_message(self): > + def test_install_nonexistent_addon(self): > addon_path = os.path.join(here, "does-not-exist.xpi") > > - with self.assertRaises(AddonInstallException) as cm: > + expected_message = "Could not find add-on at" > + with self.assertRaisesRegexp(AddonInstallException, expected_message): We shouldn't exceed the line limit when you get rid of the extra variable and directly use the string as argument.
Attachment #8911540 -
Flags: review?(hskupin) → review+
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c46a70c3439 Check that add-on file exists before forwarding request to AddonManager r=whimboo
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0c46a70c3439
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•