775580 - Remove calls to addVisit from test_sorting.js

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

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

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

Comment 1

[:Paolo Amadini]

Attachment: The patch

This includes new promiseAsyncUpdates and promiseAddVisits helpers.

Comment 2

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

Comment on attachment 643873 [details] [diff] [review]
The patch

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

Looks good to me, thanks.

Comment 3

[:Paolo Amadini]

Attachment: Updated patch

This adds the final version of the promiseAddVisits function.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[:Paolo Amadini]

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

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 8

[:Paolo Amadini]

Attachment: Revised patch

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c571638b32

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d8c571638b32