Use browser.test.assertRejects/assertThrows in browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
Categories
(WebExtensions :: General, task, P5)
Tracking
(firefox69 fixed)
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: robwu, Assigned: mihalKrasnov, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 3 obsolete files)
Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible
47 bytes,
text/x-phabricator-request
|
Details | Review |
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.
Reporter | ||
Comment 1•6 years ago
|
||
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
Assignee | ||
Comment 2•5 years ago
|
||
Started working on it, a newcomer. Looks like a lot of changes at once, should I split it into several patches?
Reporter | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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?
Reporter | ||
Comment 5•5 years ago
|
||
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.
Assignee | ||
Comment 6•5 years ago
|
||
Reporter | ||
Comment 7•5 years ago
|
||
Could you upload the patch to Phabricator for review? See https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch
Assignee | ||
Comment 8•5 years ago
|
||
Used browser.test.assertRejects to test if a Promise is rejected and browser.test.assertThrows to test if a function throws an error
Assignee | ||
Comment 9•5 years ago
|
||
(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.
Reporter | ||
Comment 10•5 years ago
|
||
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
Assignee | ||
Comment 11•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
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.
Reporter | ||
Comment 13•5 years ago
|
||
Go to Phabricator, scroll down to the comment box, click on the dropdown menu and choose "Abandon revision" to mark a patch as obsolete.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
Assignee | ||
Comment 15•5 years ago
|
||
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!
Reporter | ||
Comment 16•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 17•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment 18•5 years ago
|
||
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
Reporter | ||
Comment 19•5 years ago
|
||
Comment on attachment 9067619 [details]
Bug 1547295 - Make use of browser.test.assertRejects and browser.test.assertThrows wherever possible
(removed meaningless flag)
Comment 20•5 years ago
|
||
bugherder |
Comment 21•5 years ago
|
||
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•5 years ago
|
||
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- "
Reporter | ||
Updated•5 years ago
|
Description
•