Closed
Bug 776465
Opened 12 years ago
Closed 12 years ago
Remove calls to addVisit from the "expiration" folder
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
This removes several addVisit calls in Places tests.
Assignee | ||
Comment 1•12 years ago
|
||
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 | ||
Comment 2•12 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).
Assignee | ||
Updated•12 years ago
|
Attachment #644881 -
Flags: feedback?(mak77) → feedback?(dteller)
Updated•12 years ago
|
Attachment #644881 -
Flags: feedback?(dteller)
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #683512 -
Attachment is obsolete: true
Attachment #687067 -
Flags: feedback?(mak77)
Updated•12 years ago
|
Attachment #687067 -
Flags: feedback?(mak77) → review+
Assignee | ||
Comment 7•12 years ago
|
||
Target Milestone: --- → mozilla20
Comment 8•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•