1547295 - Use browser.test.assertRejects/assertThrows in browser/components/extensions/test/xpcshell/test_ext_bookmarks.js

Status: RESOLVED FIXED

(WebExtensions :: General, task, P5)

Description

[Rob Wu [:robwu]]

The test file at browser/components/extensions/test/xpcshell/test_ext_bookmarks.js tests whether the browser.cookies API behaves as expected.
The readability of the test can be improved by using browser.test.assertRejects and browser.test.assertThrows where possible.

Using browser.test.assertRejects where possible

There are several occurrences of the following code:

browser.bookmarks.SOMETHING(...).then(expectedError, error => {
  browser.test.assertTrue(
    error.message.includes("some error message"),
    "description of expectation");
});

// with expectedError defined as:
function expectedError() {
  browser.test.fail("Did not get expected error");
}

The test is supposed to confirm that a call to browser.bookmarks.SOMETHING rejects with an error. If the method call does unexpectedly not fail, this would fail the test with "Did not get expected error".

In our extension testing framework, we have a helper browser.test.assertRejects that was meant for this purpose, and prints a useful error message on failure. The snippet above can be rewritten to the following:

browser.test.assertRejects(
  browser.bookmarks.SOMETHING(...),
  /some error message/,
  "description of expectation");

The /some error message/ part is a regular expression to match (part of) the error message. If you want to match an exact error message, use a string instead. assertRejects is defined here and uses this errorMatches function to check the error message.

Using browser.test.assertThrows where possible

Some of the tests use the following pattern:

Promise.resolve().then(() => {
  return browser.bookmarks.SOMETHING(...);
}).then(expectedError, error => {
  browser.test.assertTrue(
    error.message.includes("some error message"),
    "description of expectation");
});

This is also meant to catch errors. Not only rejected promises, but also errors that are thrown before the browser.bookmarks.SOMETHING function even returns. This latter kind of error is thrown when the extension API is called with a value that does not match the schema definition of the cookies API.

This should be replaced with the browser.test.assertThrows:

browser.test.assertThrows(
  () => browser.bookmarks.SOMETHING(...),
  /some error message/,
  "description of expectation");

If you did that and get the failure "function did not throw, expected error", then try to use browser.test.assertRejects instead, as explained before.

Comment 1

[Rob Wu [:robwu]]

This is a good-first-bug. If you have never worked on Firefox code before, read the following guide to get started: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

This is a test-only change, so after building Firefox, you can easily check if your changes work as expected with the following command:

./mach test browser/components/extensions/test/xpcshell/test_ext_bookmarks.js

Currently the test passes. After your change, the test should still pass.

And to detect potential code style issues, run eslint:

./mach eslint browser/components/extensions/test/xpcshell/test_ext_bookmarks.js

Comment 2

[Michael Krasnov]

Started working on it, a newcomer. Looks like a lot of changes at once, should I split it into several patches?

Comment 3

[Rob Wu [:robwu]]

Understanding the bug is the most difficult part here; once you know how to fix one specific instance (given the example from the bug report), fixing all similar issues in the file becomes much easier. So you can put all changes in one patch.

Comment 4

[Michael Krasnov]

Looks like replacing Promise.resolve()... with browser.test.assertThrows(...) always results in "function did not throw, expected error". Should I proceed and just use browser.test.assertRejects everywhere?

Comment 5

[Rob Wu [:robwu]]

Use assertThrows first, and if that doesn't throw, use assertRejects without closure.

Errors caused by a mismatch of the expected parameters (as defined in schemas/menus.json) compared to the actual parameters results in a thrown error. This could be because of a type mismatch, or the use of a completely unrecognized option.

Errors thrown from parent/ext-menus.js results in an asynchronous error, caught via assertRejects.

Comment 6

[Michael Krasnov]

Attachment: Proposal to use browser.test.assertRejects and browser.test.assertThrows

Comment 7

[Rob Wu [:robwu]]

Could you upload the patch to Phabricator for review? See https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

Comment 8

[Michael Krasnov]

Attachment: Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible

Used browser.test.assertRejects to test if a Promise is rejected and browser.test.assertThrows to test if a function throws an error

Comment 9

[Michael Krasnov]

(In reply to Rob Wu [:robwu] from comment #7)

Could you upload the patch to Phabricator for review? See https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

Thank you for pointing it out, it is now uploaded to Phabricator (https://phabricator.services.mozilla.com/D32241), let me know if anything is wrong.

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 9066577 [details] [diff] [review]
Proposal to use browser.test.assertRejects and browser.test.assertThrows

Patch attachment is obsolete as it has been moved to Phabricator - https://phabricator.services.mozilla.com/D32241

Comment 11

[Michael Krasnov]

Attachment: Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible

Comment 12

[Michael Krasnov]

I think my IDE tried to fix the indentation to fix eslint errors, and messed everything up. I am going to rewrite it from scratch because the indentation is messed up now even worse. I also accidentally created a new Phabricator instance when trying to fix the issues and cannot find a way to delete it or mark obsolete.

Comment 13

[Rob Wu [:robwu]]

Go to Phabricator, scroll down to the comment box, click on the dropdown menu and choose "Abandon revision" to mark a patch as obsolete.

Comment 14

[Michael Krasnov]

Attachment: Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible

Comment 15

[Michael Krasnov]

Hi! I submitted the latest changes to Phabricator (using the same revision), but arc picked up changes for another bug I was working on (you will see a patch for browser_913972_currentset_overflow.js. So sorry for messing things up, I still can not get my head around arc, but I hope I am getting better :). Could you advise on how to remove that particular change from phabricator or submit a new patch without it? Thank you!

Comment 16

[Rob Wu [:robwu]]

arc diff squashes all commits and uploads one diff. To undo the change, just revert the last commit and run arc diff again. Are you using hg or git?

arc diff attempts to find the existing Phabricator revision in the commit message. If you want to make sure that the correct revision is updated (instead of creating a new one), specify the revision with the --update flag: arc diff --update D32623

Comment 17

[Rob Wu [:robwu]]

Michael, assuming that the test passes, the final step after the review is to add the checkin-needed keyword. You can do that by editing the bug and adding "checkin-needed" to the keyword field.

After adding the keyword, someone will eventually land your patch.

The "checkin" flag on an attachment is not actively watched, and will not result in the patch being landed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/378f90fe8bfa
Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible r=robwu,rpl

Comment 19

[Rob Wu [:robwu]]

Comment on attachment 9067619 [details]
Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible

(removed meaningless flag)

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/378f90fe8bfa

Comment 21

[Caitlin Neiman [:caitmuenster]]

Thanks so much for the patch, Michael! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!

Comment 22

[Madalin Cotetiu]

From my understanding this issue is related to the addon tests. Is there any need of manual QA here? If not can you please mark it as "qe-verify- "