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

Status: RESOLVED FIXED

Description

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 1

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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 2

[Ian MacLeod (:skeletor)]

Created attachment 8911540 [details]
Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager

Review commit: https://reviewboard.mozilla.org/r/182982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182982/

Comment 3

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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".

Comment 4

[Ian MacLeod (:skeletor)]

Comment on attachment 8911540 [details]
Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182982/diff/1-2/

Comment 5

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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?

Comment 6

[Ian MacLeod (:skeletor)]

Comment on attachment 8911540 [details]
Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182982/diff/2-3/

Comment 7

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 8

[Ian MacLeod (:skeletor)]

Comment on attachment 8911540 [details]
Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182982/diff/3-4/

Comment 9

[Ian MacLeod (:skeletor)]

Comment on attachment 8911540 [details]
Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182982/diff/4-5/

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 11

[Ian MacLeod (:skeletor)]

Comment on attachment 8911540 [details]
Bug 1389488 - Check that add-on file exists before forwarding request to AddonManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182982/diff/5-6/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c46a70c3439
Check that add-on file exists before forwarding request to AddonManager r=whimboo

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c46a70c3439