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)
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•8 years ago
|
Priority: -- → P3
Reporter | ||
Comment 1•8 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•8 years ago
|
Attachment #8911540 -
Flags: review?(hskupin)
Reporter | ||
Comment 3•8 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•8 years ago
|
Assignee: nobody → rubberdonkeysandwich
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•