Closed Bug 1389488 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Marionette, enhancement, P3)

Product:
Testing
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: 4 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.