Closed Bug 1389488 Opened 8 years ago Closed 8 years ago

addon.install should check if path of extension exists for a better error message

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
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)

Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager
59 bytes, text/x-review-board-request
whimboo
: review+
Details
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.
Priority: -- → P3
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 ...".
Mentor: hskupin
User Story: (updated)
Keywords: good-first-bug
Whiteboard: [lang=js][lang=py]
Attachment #8911540 - Flags: review?(hskupin)
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+
Assignee: nobody → rubberdonkeysandwich
Status: NEW → ASSIGNED
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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/0c46a70c3439
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: