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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing the issue

Comment 2

[Marco Bonardo [:mak]]

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?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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

[Marco Bonardo [:mak]]

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 :)

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2437fe7ca725

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2437fe7ca725