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

RESOLVED FIXED in Firefox 69

Status

task
P5
normal
RESOLVED FIXED
4 months ago
Last month

People

(Reporter: robwu, Assigned: mihalKrasnov, Mentored)

Tracking

({good-first-bug})

66 Branch
mozilla69
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.

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

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

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.

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?

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)

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

Assignee: nobody → mihalKrasnov
Status: NEW → ASSIGNED
Flags: needinfo?(mihalKrasnov)

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

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

Flags: needinfo?(mihalKrasnov)
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
Attachment #9066577 - Attachment is obsolete: true
Attachment #9066577 - Flags: review?(rob)
Attachment #9067534 - Attachment description: Update patch → Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible

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.

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

Attachment #9066897 - Attachment is obsolete: true
Attachment #9067534 - Attachment is obsolete: true

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!

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

Attachment #9067619 - Flags: checkin+

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.

Keywords: checkin-needed

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/378f90fe8bfa
Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible r=robwu,rpl

Keywords: checkin-needed

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

(removed meaningless flag)

Attachment #9067619 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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!

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

Flags: needinfo?(rob)
Flags: needinfo?(rob) → qe-verify-
You need to log in before you can comment on or make changes to this bug.