Closed Bug 1278538 Opened 6 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Iteration:
50.1 - Jun 20
Tracking Status
firefox50 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [history]triaged)

Attachments

(1 file)

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
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Priority: -- → P3
Whiteboard: [history]triaged
The changes to migrate test_search from addVisits to insertMany is being done via bug 1271675.
Depends on: 1271675
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...
Attachment #8760778 - Flags: review?(aswan) → review+
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/
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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5adb34ff698d
Migrate tests for browser.history to use History.insertMany rather than PlacesTestUtils.addVisits, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5adb34ff698d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.