776465 - Remove calls to addVisit from the "expiration" folder

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

This removes several addVisit calls in Places tests.

Comment 1

[:Paolo Amadini]

Attachment: The pacth

This patch keeps callbacks in some trivial test cases, and uses Tasks where
it's simpler than callbacks. The patch might seem long, because some code
blocks are now moved to logical order and changed indentation, but the changes
to actual code are in fact minimal, only a few lines per test.

Following this minimal changes approach, when replacing addVisit with
promiseAddVisits I've just done it line by line, without making any
optimization and grouping, so that the changes were very fast to make.

Comment 2

[:Paolo Amadini]

Two notes: the visit transition type in these tests is not relevant, and
promiseTopicObserved is defined in another patch (but should be self-explanatory).

Comment 3

[:Paolo Amadini]

Attachment: Updated patch

This patch contains the first relevant test rewritings. I've rewritten tests
based on opportunity, converting them to "add_task" when it was so easy that it
didn't make sense to keep an existing local, custom test harness. In other cases
I've kept the existing local runner, when converting was just a lot of work.

If this strategy works for you, I'll continue for the other patches as well.

Comment 4

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

Comment on attachment 683512 [details] [diff] [review]
Updated patch

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

No blocking issues, just the gTests, gCurrentTest renaming should be done coherently (everywhere or nowhere)

::: toolkit/components/places/tests/expiration/test_annos_expire_policy.js
@@ +161,5 @@
>    }
>  
> +  // Expire all visits for the bookmarks.
> +  yield promiseForceExpirationStep(5);
> +  

nit: trailing spaces

::: toolkit/components/places/tests/expiration/test_pref_interval.js
@@ +131,5 @@
>    do_test_pending();
>  }
>  
>  function run_next_test() {
>    if (gTests.length) {

in most other tests you renamed gTests to tests... I don't remember if this was done to add support for add_task... likely we may want to use less generic names in the "harness" and be coherent with replacements where needed, either everywhere or nowhere

::: toolkit/components/places/tests/expiration/test_removeAllPages.js
@@ +129,5 @@
> +  // Expire all visits for the bookmarks.
> +  let promise =
> +      promiseTopicObserved(PlacesUtils.TOPIC_EXPIRATION_FINISHED);
> +  hs.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
> +  yield promise;

This really sounds to me like "yield promiseClearHistory();"

Comment 5

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #4)
> ::: toolkit/components/places/tests/expiration/test_removeAllPages.js
> @@ +129,5 @@
> > +  // Expire all visits for the bookmarks.
> > +  let promise =
> > +      promiseTopicObserved(PlacesUtils.TOPIC_EXPIRATION_FINISHED);
> > +  hs.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
> > +  yield promise;
> 
> This really sounds to me like "yield promiseClearHistory();"

In general, I've avoided using helpers in tests that are explicitly exercising
a given function used in the helper (both for demonstration purposes, and just
in case the helper is rewritten later to use a different function). I've added
a comment to that effect.

Comment 6

[:Paolo Amadini]

Attachment: Revised patch

Comment 7

[:Paolo Amadini]

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

Comment 8

[(no longer active)]

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