Closed Bug 1010255 Opened 7 years ago Closed 7 years ago

TEST-UNEXPECTED-FAIL | resource://gre/modules/BookmarkHTMLUtils.jsm | A promise chain failed to handle a rejection: Error: Cannot import from nonexisting html file

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file)

No description provided.
Attached patch Fixing the issueSplinter Review
Assignee: nobody → dteller
Attachment #8422461 - Flags: review?(mak77)
Comment on attachment 8422461 [details] [diff] [review]
Fixing the issue

Review of attachment 8422461 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js
@@ +151,5 @@
>      currTopic:  NSIOBSERVER_TOPIC_BEGIN,
>      finalTopic: NSIOBSERVER_TOPIC_FAILED,
>      data:       NSIOBSERVER_DATA_HTML,
>      folderId:   null,
> +    run:        Task.async(function* () {

nobody is yielding run() so this looks more confusing than the old code, and make it look like one can use a task in run() (ideally that would be nice but for that we should refactor the test)
what about just adding a silent failure handler along with the success handler in the then?
(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 8422461 [details] [diff] [review]
> Fixing the issue
> 
> Review of attachment 8422461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js
> @@ +151,5 @@
> >      currTopic:  NSIOBSERVER_TOPIC_BEGIN,
> >      finalTopic: NSIOBSERVER_TOPIC_FAILED,
> >      data:       NSIOBSERVER_DATA_HTML,
> >      folderId:   null,
> > +    run:        Task.async(function* () {
> 
> nobody is yielding run() so this looks more confusing than the old code, and
> make it look like one can use a task in run() (ideally that would be nice
> but for that we should refactor the test)
> what about just adding a silent failure handler along with the success
> handler in the then?

Well, all the other instances of `run` in that test are tasks. Granted, it's
  run: function() { Task.spawn(function*() { ... })
instead of
  run: Task.async(function*() { ... })
which is slightly different. If you wish, I could use the Task.spawn version, but using Promise just for these two samples doesn't feel right.
Comment on attachment 8422461 [details] [diff] [review]
Fixing the issue

Review of attachment 8422461 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 8th-12th) from comment #3)
> Well, all the other instances of `run` in that test are tasks. Granted, it's
>   run: function() { Task.spawn(function*() { ... })
> instead of
>   run: Task.async(function*() { ... })
> which is slightly different. If you wish, I could use the Task.spawn
> version, but using Promise just for these two samples doesn't feel right.

That's because the test uses a nsIObserver topic to proceed. This test was modified when we moved backups to async, and that's how it grew those Task calls, it should have been refactored to make run a Task and have a yield promiseTopic inside the tasks... But that didn't happen, so it was just modified to use Task here and there.

I'm happy to take your patch if you file a follow-up mentored good first bug to refactor the test to properly use Tasks in run(), this is a condicio sine qua non :)
Attachment #8422461 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/2437fe7ca725
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.