Complete test coverage for the WebExtension bookmarks API.

RESOLVED FIXED in Firefox 48

Status

P3
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: kmag, Assigned: bsilverberg)

Tracking

unspecified
mozilla48
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [test] triaged)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This is still missing coverage for:

* The |getSubTree| and |move| API methods.
* The |getChildren| and |getTree| methods where the promise is rejected.
* The |create| method with an |index| property.
* The |update| method with a |url| property, or a rejected promise.


https://people.mozilla.org/~kmaglione/webextension-test-coverage/browser/components/extensions/ext-bookmarks.js.html

Updated

3 years ago
Flags: blocking-webextensions?
Priority: -- → P3
Whiteboard: [test] triaged

Updated

3 years ago
Flags: blocking-webextensions? → blocking-webextensions+
(Assignee)

Comment 1

3 years ago
I'll work on this next.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Iteration: --- → 47.3 - Mar 7
(Assignee)

Updated

3 years ago
Depends on: 1251244
(Assignee)

Comment 2

3 years ago
Note to self: getSubTree coverage landed in https://hg.mozilla.org/mozilla-central/rev/23566cc859df
(Assignee)

Updated

3 years ago
Depends on: 1253652
(Assignee)

Updated

3 years ago
Iteration: 47.3 - Mar 7 → 48.1 - Mar 21
(Assignee)

Comment 3

3 years ago
move() coverage is covered in bug 1253652
(Assignee)

Comment 4

3 years ago
Created attachment 8727917 [details]
MozReview Request: Bug 1236121 - Complete test coverage for the WebExtension bookmarks API, r?kmag

Add coverage for getChildren() when the promise is rejected
Add coverage for create() specifying index
Add coverage for update() specifying a URL
Add coverage for update() when the promise is rejected

Review commit: https://reviewboard.mozilla.org/r/38681/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38681/
Attachment #8727917 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

3 years ago
I believe that the attached patch, plus bug 1253652 should complete the coverage for bookmarks.
(Reporter)

Comment 6

3 years ago
Comment on attachment 8727917 [details]
MozReview Request: Bug 1236121 - Complete test coverage for the WebExtension bookmarks API, r?kmag

https://reviewboard.mozilla.org/r/38681/#review35417

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:167
(Diff revision 1)
> +        parentId: results[0].parentId,

I don't understand why you're using `results[0].parentId` for this.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:170
(Diff revision 1)
> +    }).then(results => {

`results` doesn't seem like an appropriate name for this argument.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:288
(Diff revision 1)
> -    browser.test.assertEq(3, results.length, "Expected number of multiple results returned");
> +    dump(JSON.stringify(results, null, 4));

Please remove.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:394
(Diff revision 1)
> +  }).then(expectedError, error => {

This handler needs to be attached directly to `getChildren` so it doesn't catch errors from the rest of the chain.
Attachment #8727917 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 7

3 years ago
Comment on attachment 8727917 [details]
MozReview Request: Bug 1236121 - Complete test coverage for the WebExtension bookmarks API, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38681/diff/1-2/
(Assignee)

Comment 8

3 years ago
https://reviewboard.mozilla.org/r/38681/#review35417

> I don't understand why you're using `results[0].parentId` for this.

I think that was leftover from when I structured it differently. You're right, I don't need to do that anymore.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/571889335aa4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.