Remove calls to addVisit from test_sorting.js

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
This "pilot" bug removes one instance of nested asynchronous addVisit calls.

The same approach could be applied in various other places as well.
(Assignee)

Comment 1

6 years ago
Created attachment 643873 [details] [diff] [review]
The patch

This includes new promiseAsyncUpdates and promiseAddVisits helpers.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #643873 - Flags: feedback?(mak77)

Updated

6 years ago
Blocks: 700250
(Assignee)

Updated

6 years ago
Attachment #643873 - Flags: feedback?(mak77) → feedback?(dteller)
Attachment #643873 - Flags: feedback?(dteller)
(Assignee)

Updated

6 years ago
Blocks: 776465
Depends on: 763311
Comment on attachment 643873 [details] [diff] [review]
The patch

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

Looks good to me, thanks.
Attachment #643873 - Flags: feedback+
(Assignee)

Comment 3

6 years ago
Created attachment 683501 [details] [diff] [review]
Updated patch

This adds the final version of the promiseAddVisits function.
Attachment #643873 - Attachment is obsolete: true
Attachment #683501 - Flags: review?(mak77)
Comment on attachment 683501 [details] [diff] [review]
Updated patch

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

::: toolkit/components/places/tests/head_common.js
@@ +942,5 @@
> + * on success, or reports a test error on failure.
> + *
> + * @see promiseAddVisits
> + */
> +function addVisits(aPlaceInfo, aCallback, aStack)

please file a follow-up bug to get rid of this duplicated helper, we should just switch everything to the promiseAddVisits version.

Add a "@note deprecated, please use promiseAddVisits instead."

::: toolkit/components/places/tests/queries/test_sorting.js
@@ +1267,2 @@
>        // sorting reversed, usually SORT_BY have ASC and DESC
> +      yield test.check_reverse();

most setup(), all of check() and all of check_reverse() calls are synchronous and don't return anything, doesn't yield require to get a promise? Is this voodoo magic from Task?
(In reply to Marco Bonardo [:mak] from comment #4)
> most setup(), all of check() and all of check_reverse() calls are
> synchronous and don't return anything, doesn't yield require to get a
> promise? Is this voodoo magic from Task?

OK I think I got an answer by the fact Task just continues if yield is not a promise. Though looks strange to use it in check and check_reverse, I couldn't find an async case there.

Updated

6 years ago
Attachment #683501 - Flags: review?(mak77)
(Assignee)

Comment 6

6 years ago
(In reply to Marco Bonardo [:mak] from comment #5)
> OK I think I got an answer by the fact Task just continues if yield is not a
> promise. Though looks strange to use it in check and check_reverse, I
> couldn't find an async case there.

Yeah, this is done on purpose (see the Task.jsm documentation) to avoid pitfalls
that may happen when you batch-remove a call that is not needed anymore, and
doing that you also remove the last yield call. For example, before:

  add_task(function test() {
    yield prepareLegacyDataStructures();
    do_check_true(featureIsOk);
  });

And after:

  add_task(function test() {
    do_check_true(featureIsOk);
  });

This is trivial here, but may not be so obvious in longer test functions. In
this case it's the other way around, in case you need to add a yield statement.
(In reply to Paolo Amadini [:paolo] from comment #6)
> (In reply to Marco Bonardo [:mak] from comment #5)
> > OK I think I got an answer by the fact Task just continues if yield is not a
> > promise. Though looks strange to use it in check and check_reverse, I
> > couldn't find an async case there.
> 
> Yeah, this is done on purpose (see the Task.jsm documentation) to avoid
> pitfalls
> that may happen when you batch-remove a call that is not needed anymore, and
> doing that you also remove the last yield call. For example, before:

the test timeouts in such a case, doesn't it? So it's a detectable issue once you do the change and see the test fails.
(Assignee)

Comment 8

6 years ago
Created attachment 687066 [details] [diff] [review]
Revised patch
Attachment #683501 - Attachment is obsolete: true
Attachment #687066 - Flags: review?(mak77)

Updated

6 years ago
Attachment #687066 - Flags: review?(mak77) → review+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c571638b32
Target Milestone: --- → mozilla20

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/d8c571638b32
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.