1278538 - Migrate tests for browser.history to use History.insertMany rather than PlacesTestUtils.addVisits

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

In bug 1272686 we are changing the internal implementation of PlacesTestUtils.addVisits [1] to use History.insertMany [2]. This requires changes to some of the tests that call PlacesTestUtils.addVisits, so rather than simply making the changes to allow us to keep using PlacesTestUtils.addVisits, it makes sense to migrate to History.insertMany which has a more friendly API.

This bug is to migrate all of the tests in browser_ext_history.js [3].

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/PlacesTestUtils.jsm#39
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.jsm#231
[3] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_history.js

Comment 1

[Bob Silverberg [:bsilverberg]]

The changes to migrate test_search from addVisits to insertMany is being done via bug 1271675.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1278538 - Migrate tests for browser.history to use History.insertMany rather than PlacesTestUtils.addVisits,

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8760778 [details]
Bug 1278538 - Migrate tests for browser.history to use History.insertMany rather than PlacesTestUtils.addVisits,

https://reviewboard.mozilla.org/r/58228/#review55288

::: browser/components/extensions/test/browser/browser_ext_history.js:66
(Diff revision 1)
>    let historyClearedCount;
>    let visits = [];
> +  let visitDate = new Date();
>  
> -  // Add 5 visits for one uri and 3 visits for 3 others
> -  for (let i = 0; i < 8; ++i) {
> +  function pushVisit(visits) {
> +    visitDate = new Date(Number(visitDate) - 1000);

nitpick, why don't you keep visitDate as a number and call `new Date()` when you push onto the array?

::: browser/components/extensions/test/browser/browser_ext_history.js:99
(Diff revision 1)
> -    startTime: PlacesUtils.toDate(visits[1].visitDate),
> -    endTime: PlacesUtils.toDate(visits[3].visitDate),
> +    startTime: visits[3].visits[4].date,
> +    endTime: visits[3].visits[2].date,

The logic here seems correct but the new-to-old ordering makes it confusing...

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8760778 [details]
Bug 1278538 - Migrate tests for browser.history to use History.insertMany rather than PlacesTestUtils.addVisits,

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

Comment 5

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/58228/#review55288

> The logic here seems correct but the new-to-old ordering makes it confusing...

I agree. I found it very confusing when rewriting this. I don't know why I decided to do everything backwards! I've fixed that.

Comment 6

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=933f5a095f4f

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5adb34ff698d
Migrate tests for browser.history to use History.insertMany rather than PlacesTestUtils.addVisits, r=aswan

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5adb34ff698d