Allow for sharing of Places test utils that are currently in head_common.js

RESOLVED FIXED in Firefox 49

Status

()

Toolkit
Places
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
I am working on the new web extensions History API and for some of the tests I am writing I would like to have access to some of the helper functions that are currently defined in head_common.js, specifically `page_in_database` [1] and `visits_in_database` [2].

It was suggested that the best way to share these would be for them to be in a JS module, and PlacesTestUtils.jsm [3] seems like the ideal candidate.

Those two functions require access to `DBConn` [4], so this would mean moving that as well. A lot of existing tests are utilizing those three functions via head_common.js, but I think we can avoid having to change all of those tests (if that's something we'd want to avoid) by simply changing the code in head_common.js to utilize the code from PlacesTestUtils.jsm after the functions have been moved.

I can do the work to move the functions into PlacesTestUtils.jsm and either update head_common.js or the tests that currently use the functions, but:

a) Is this something that would be a good idea to do?
b) If not, how would you suggest that our tests get access to the logic of `page_in_database` and `visits_in_database`?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#297
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#320
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/PlacesTestUtils.jsm
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#102
(Assignee)

Updated

2 years ago
Flags: needinfo?(mak77)
(Assignee)

Comment 1

2 years ago
Created attachment 8745322 [details]
MozReview Request: Bug 1267517 - Allow for sharing of Places test utils that are currently in head_common.js, r?mak

Review commit: https://reviewboard.mozilla.org/r/48979/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48979/
Attachment #8745322 - Flags: review?(mak77)
(Assignee)

Comment 2

2 years ago
I decided to just go ahead and create the patch, hoping that it will be acceptable. I also didn't go with the quick and dirty approach of proxying the methods through head_common.js and simply changed all of the code that calls `DBConn`, `page_in_database` and `visits_in_database` as there wasn't really that much of it.
(Assignee)

Updated

2 years ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Yes, in general it's a good idea to move useful methods into PlacesTestUtils and usually while we do that, we also clean and modernize their code/signature.
Flags: needinfo?(mak77)
(Assignee)

Comment 4

2 years ago
Great, thanks Mak. :)  Please let me know via the review if you think anything about these methods should be changed, or even if you think there are other methods that should be moved into PlacesTestUtils while I'm doing this. It might be more efficient to do any that need to be done now, rather than via a separate patch later.
Comment on attachment 8745322 [details]
MozReview Request: Bug 1267517 - Allow for sharing of Places test utils that are currently in head_common.js, r?mak

https://reviewboard.mozilla.org/r/48979/#review45825

::: toolkit/components/places/tests/PlacesTestUtils.jsm:153
(Diff revision 1)
> +   *        connection is asyncClosed it cannot anymore schedule async statements,
> +   *        though connectionReady will keep returning true (Bug 726990).
> +   *
> +   * @return The database connection or null if unable to get one.
> +   */
> +  DBConn(aForceNewConnection) {

For now I'd prefer to not move the DBConn() helper, cause there are only a few tests that will actuall need to create a connection from scratch, and I'd like to identify those better by providing a separate helper...

I think for the cases you care about it will be fine to just use PlacesUtils.history.DBConnection (you don't need the additional QI)

You are clearly not required to fix every single call point, the remaining tests using DBConn() could be converted apart.

::: toolkit/components/places/tests/PlacesTestUtils.jsm:184
(Diff revision 1)
> +   * Checks if an address is found in the database.
> +   * @param aURI
> +   *        nsIURI or address to look for.
> +   * @return place id of the page or 0 if not found
> +   */
> +  page_in_database(aURI) {

In general, I'd prefer if utils exposed by PlacesTestUtils would be async and promise-based (unless they don't require I/O).
And that they respect the usual cameCase style of the other methods already there.

So these could be:

isPageInDatabase and hasVisitInDatabase

this may require to use add_task and some yielding, so again, you are not expected to rewrite those tests that are still synchronous. It can happen apart.

For these I'd suggest to make them Task.async and use PlacesUtils.promiseDBConnection as connection.
Attachment #8745322 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #5)
> hasVisitInDatabase

hasVisitsInDatabase (plural) is probably better... I'd also be fine with shrinking Database with just DB
(Assignee)

Comment 7

2 years ago
Comment on attachment 8745322 [details]
MozReview Request: Bug 1267517 - Allow for sharing of Places test utils that are currently in head_common.js, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48979/diff/1-2/
Attachment #8745322 - Flags: review?(mak77)
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/48979/#review45825

> In general, I'd prefer if utils exposed by PlacesTestUtils would be async and promise-based (unless they don't require I/O).
> And that they respect the usual cameCase style of the other methods already there.
> 
> So these could be:
> 
> isPageInDatabase and hasVisitInDatabase
> 
> this may require to use add_task and some yielding, so again, you are not expected to rewrite those tests that are still synchronous. It can happen apart.
> 
> For these I'd suggest to make them Task.async and use PlacesUtils.promiseDBConnection as connection.

I assume this means that I need to leave the existing methods in `head_common.js` so they can continue to be used by the existing tests, so that's what I've done.

Also, I called the new method `visitsInDB` as opposed to `hasVisitsInDB` as it returns a count of the number of visits, not just a boolean. I also added tests for the two new methods, although I'm not sure if I've put them in the correct place.
Comment on attachment 8745322 [details]
MozReview Request: Bug 1267517 - Allow for sharing of Places test utils that are currently in head_common.js, r?mak

https://reviewboard.mozilla.org/r/48979/#review46099

::: toolkit/components/places/tests/unit/test_isPageInDB.js:5
(Diff revision 2)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: FYI, in test code you can avoid the license header, and they will default to a PD license

::: toolkit/components/places/tests/unit/test_isPageInDB.js:10
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function run_test()
> +{
> +  run_next_test();
> +}

run_test is no more needed whtn it only invokes run_next_test

::: toolkit/components/places/tests/unit/test_isPageInDB.js:13
(Diff revision 2)
> +{
> +  run_next_test();
> +}
> +
> +add_task(function* test_execute()
> +{

nit: I know in some tests we are using this style, for new code I'd prefer if we'd stick to the most common style that is to have the brace on the same line as the function declaration.

::: toolkit/components/places/tests/unit/test_visitsInDB.js:10
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function run_test()
> +{
> +  run_next_test();
> +}

ditto
Attachment #8745322 - Flags: review?(mak77) → review+
(Assignee)

Comment 10

2 years ago
Comment on attachment 8745322 [details]
MozReview Request: Bug 1267517 - Allow for sharing of Places test utils that are currently in head_common.js, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48979/diff/2-3/
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcae9847cd63
(Assignee)

Comment 12

2 years ago
Thanks for the review and the tips, Marco!
Feel free to add any more utils you need to that module. Sooner or later most of head_common should move there and slowly we'll refactor the tests (or ask the community to help with that).
(Assignee)

Updated

2 years ago
Iteration: --- → 49.1 - May 9
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1265842

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/0a7afb861f54
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a7afb861f54
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.