1271675 - Add tests for browser.history.search that use specific date ranges

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

Once `insert` lands in History.jsm (which is being implemented as part of bug 1265836) we can (and should) enhance the tests for history.search to add history with specific dates (via History.insert) and then search for specific date ranges.

Currently the tests for history.search only include tests which return no results for dates that are too early or too late as we don't have an easy way to add history using anything by the current time.

Comment 1

[Bob Silverberg [:bsilverberg]]

Because bug 1265835 is using the same set of tests as this bug references, and because getVisits will also need test data with specific dates which will be added via History.insert, I am combining this bug with that one. It will become Part 1 of that bug.

Comment 2

[Bob Silverberg [:bsilverberg]]

After spending a bunch of time trying to create tests for both search and getVisits I've realized that I'm just over complicating things by trying to do that, and ending up with a test that's far less readable, so I'm reopening this to address it separately from bug 1265835.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1271675 - Add tests for browser.history.search that use specific date ranges,

Review commit: https://reviewboard.mozilla.org/r/57968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57968/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8760396 [details]
Bug 1271675 - Add tests for browser.history.search that use specific date ranges,

https://reviewboard.mozilla.org/r/57968/#review55286

The bug title says "add tests" but the patch only adds one test :-)

::: browser/components/extensions/test/browser/browser_ext_history.js:219
(Diff revision 1)
> -  is(results.length, 1, "history.search returned 1 result");
> +  is(results.length, 1, "history.search with maxResults returned 1 result");
>    checkResult(results, DOUBLE_VISIT_URL, 2);
>  
> +  results = yield extension.awaitMessage("date-range-search");
> +  is(results.length, 2, "history.search with a date range returned 2 result");
> +  checkResult(results, DOUBLE_VISIT_URL, 2);

why is the count here 2?  there are two visits but only one is in the range that you're searching for.

Comment 5

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/57968/#review55286

Yeah, after writing the one I decided that it was enough to cover the functionality.

> why is the count here 2?  there are two visits but only one is in the range that you're searching for.

`history.search` returns `HistoryItem`s [1], not `VisitItem`s [2]. This means that the criteria is used to return a HistoryItem for any URL that was visited during the date range. Each HistoryItem, however, will contain a count of the number of times that the URL was visited *for all time*. It is a bit confusing, but that's how it works. Whenever a HistoryItem is returned, it always contains a visitCount property which is the count of *all* visits to that URL.

[1] https://developer.chrome.com/extensions/history#type-HistoryItem
[2] https://developer.chrome.com/extensions/history#type-VisitItem

Comment 6

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b93f9323cd1

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8760396 [details]
Bug 1271675 - Add tests for browser.history.search that use specific date ranges,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57968/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f8bdceb55c9d
Add tests for browser.history.search that use specific date ranges. r=aswan

Comment 9

[Carsten Book [:Tomcat]]

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