Remove calls to addVisit from the "expiration" folder

RESOLVED FIXED in mozilla20

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
5 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 removes several addVisit calls in Places tests.
(Assignee)

Comment 1

6 years ago
Created attachment 644881 [details] [diff] [review]
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #644881 - Flags: feedback?(mak77)
(Assignee)

Comment 2

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

Updated

6 years ago
Blocks: 700250
(Assignee)

Updated

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

Updated

6 years ago
Depends on: 775580
(Assignee)

Updated

6 years ago
Blocks: 776855
(Assignee)

Comment 3

5 years ago
Created attachment 683512 [details] [diff] [review]
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.
Attachment #644881 - Attachment is obsolete: true
Attachment #683512 - Flags: review?(mak77)
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();"
Attachment #683512 - Flags: review?(mak77) → review+
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 687067 [details] [diff] [review]
Revised patch
Attachment #683512 - Attachment is obsolete: true
Attachment #687067 - Flags: feedback?(mak77)

Updated

5 years ago
Attachment #687067 - Flags: feedback?(mak77) → review+
(Assignee)

Comment 7

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

Comment 8

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