Once `insertMany` lands in History.jsm, change the internal implementation of PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany.

Marco, I've started working on this and I notice that the current API for `addVisits` accepts what is referred to as a `PlaceInfo` as opposed to a `PageInfo` which results in a mismatch between `visitDate` [1] and `date` [2]. That is, when calling addVisits, one can provide a `visitDate`, but when calling `insertMany` one must use `date`. I can keep the current API for `addVisits` the same, which wouldn't cause any backward compatibility issues, and do the translation inside `addVisits` but it strikes me as unfortunate that the arguments won't match. We can also change the API for `addVisits` to expect a PageInfo object, but that would mean potentially changing hundreds of calls that currently exist for the method. Therefore I'm inclined to just keep the (now) nonstandard API the same. What do you think I should do? [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/PlacesTestUtils.jsm#31 [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.jsm#40

Note that the implementation of PlacesUtils.history.insertMany has not landed yet, but I had some time to work on this so I did it. Submitting for review so that it will be ready to land after insertMany lands. I have addressed all of the failures in tests in toolkit/components/places/ that this change caused, but there may be tests in other folders which also need to be fixed. I'm going to submit a try to see what else shows up. Review commit: https://reviewboard.mozilla.org/r/53330/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53330/

Comment on attachment 8753845 [details] Bug 1272686 - Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53330/diff/1-2/

Comment on attachment 8753845 [details] Bug 1272686 - Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53330/diff/2-3/

Marco, I've been looking through the failure on try to see what other tests might need to be fixed as a result of this change. I found test_quota_with_notification.js which is failing because it tries to add a visit that is in the future. We currently prevent that from happening, but this test seems to think it needs to do that. I can follow up with the author of the test (if I can find him/her) but do we need to rethink that decision? Might there be times when we need to add a visit that is in the future, or is that never a valid thing to do? [1] https://dxr.mozilla.org/mozilla-central/source/dom/push/test/xpcshell/test_quota_with_notification.js#64

Kit, it looks like you added the code to test_quota_with_notification.js (see comment 7, above) that inserts a visit into Places with a future date. We are changing the API for addVisits to prevent future-dated visits from being added, as that is considered to be invalid. Is there a way for you to change your test so it does not need a future-dated visit added? Why is it that you need to add a visit with a date in the future for this test?

I added that test because we noticed some rare cases where the last visit date was > `Date.now()`, causing intermittent failures on one of the push tests (bug 1206424). But there are better ways to test quota recalculation than inserting a visit in the future. I'll try to get to this today. If you'd like to land your patch now, though, please feel free to disable test_quota_with_notification.js until I fix it.

Thanks Kit. I appreciate you offering to change the test. This change is not likely to land before next week, so there's no huge rush. If you're going to open a bug for the change to test_quota_with_notification.js could you please set it up to block this bug? Or I can create the bug for you if you like - just let me know in which component to file it.

(In reply to Bob Silverberg [:bsilverberg] from comment #1) > What do you think I should do? It really depends on your availability. The best thhing would be clearly to migrate everything to the new signature. But you're right, it's a lot of consumers and it's going to require a huge effort to convert everyone. since in the end the scope here is to increase test coverage of history.insert, I think we should do the internal conversion for now.

Comment on attachment 8753845 [details] Bug 1272686 - Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, https://reviewboard.mozilla.org/r/53330/#review50940 ::: toolkit/components/places/tests/PlacesTestUtils.jsm:62 (Diff revision 3) > for (let place of places) { > + let info = {url: place.uri}; > if (typeof place.uri == "string") { > - place.uri = NetUtil.newURI(place.uri); > + info.url = NetUtil.newURI(place.uri); > } else if (place.uri instanceof URL) { > - place.uri = NetUtil.newURI(place.href); > + info.url = NetUtil.newURI(place.href); don't we already make this conversion in insert itself? It looks pointless now. ::: toolkit/components/places/tests/PlacesTestUtils.jsm:67 (Diff revision 3) > - place.title = "test visit for " + place.uri.spec; > } > - place.visits = [{ > - transitionType: place.transition === undefined ? Ci.nsINavHistoryService.TRANSITION_LINK > - : place.transition, > - visitDate: place.visitDate || (now++) * 1000, > + info.title = (typeof place.title === "string") ? place.title : "test visit for " + info.url.spec ; > + info.visits = [{ > + transition: place.transition, > + date: (place.visitDate) ? PlacesUtils.toDate(place.visitDate) : new Date(), the parenthesis don't look useful Maybe we could do something here to make the APIs closer, like accepting either a microtime or a Date object. (Ideally we could even distinguish microtime from timestamp by comparing with a threshold date, but I feel like that would be very hacky). ::: toolkit/components/places/tests/queries/test_history_queries_tags_liveUpdate.js:7 (Diff revision 3) > http://creativecommons.org/publicdomain/zero/1.0/ */ > > // This test ensures that tags changes are correctly live-updated in a history > // query. > > -var gNow = Date.now(); > +let timeInMicroseconds = (Date.now() - 10000) * 1000; PlacesUtils.toPRTime ::: toolkit/components/places/tests/queries/test_history_queries_titles_liveUpdate.js:7 (Diff revision 3) > http://creativecommons.org/publicdomain/zero/1.0/ */ > > // This test ensures that tags changes are correctly live-updated in a history > // query. > > -var gNow = Date.now(); > +let timeInMicroseconds = (Date.now() - 10000) * 1000; toPRTime ::: toolkit/components/places/tests/queries/test_results-as-visit.js:7 (Diff revision 3) > /* 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/. */ > var testData = []; > -var now = Date.now() * 1000; > +var timeInMicroseconds = (Date.now() - 10000) * 1000; toPRTime ::: toolkit/components/places/tests/queries/test_searchterms-uri.js:39 (Diff revision 3) > {isInQuery: false, isVisit:true, isDetails: true, title: "m%0o%0z", > uri: "http://foo.com/changeme1.htm", lastVisit: yesterday}, > > // Test another invalid title - for updating later > {isInQuery: false, isVisit:true, isDetails: true, title: "m,oz", > - uri: "http://foo.com/changeme2.htm", lastVisit: tomorrow}]; > + uri: "http://foo.com/changeme2.htm", lastVisit: yesterday}]; can tomorrow be removed from head_queries or is it still used anywhere? ::: toolkit/components/places/tests/queries/test_sorting.js:1192 (Diff revision 3) > _sortingMode: Ci.nsINavHistoryQueryOptions.SORT_BY_FRECENCY_ASCENDING, > > *setup() { > do_print("Sorting test 13: SORT BY FRECENCY "); > > - var timeInMicroseconds = Date.now() * 1000; > + let timeInMicroseconds = (Date.now() - 10000) * 1000; toPRTime ::: toolkit/components/places/tests/unit/test_420331_wyciwyg.js:10 (Diff revision 3) > -function run_test() > -{ > +add_task(function* test_execute() { > + var testUri = uri("wyciwyg://nodontjudgeabookbyitscover"); > - run_next_test(); > -} > > -add_task(function* test_execute() > + PlacesTestUtils.addVisits(testUri).then(() => { Is this test already covered by test_isvisited.js?

Is this blocked on something I can help with?

Thanks for checking in, Marco. No, it's just me having other higher priorities. I need to address your review comments, and double check for any tests in-tree that this may have broken. I should be able to get back to it this week or next. Is that okay?

Sure, no problem, was just guessing whether my comments were unclear to help with that.

Comment on attachment 8753845 [details] Bug 1272686 - Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, https://reviewboard.mozilla.org/r/53330/#review50940 I'm sorry to have taken sooooo long to get back to this Marco. I have addressed your comments and now I will submit this to try again to see if there are any remaining test failures. > don't we already make this conversion in insert itself? It looks pointless now. Good point. Fixed. > the parenthesis don't look useful > > Maybe we could do something here to make the APIs closer, like accepting either a microtime or a Date object. > (Ideally we could even distinguish microtime from timestamp by comparing with a threshold date, but I feel like that would be very hacky). I removed the parentheses and changed the `addVisits` to accept a date object as well as a microtime (which I think is what you were suggesting).

Comment on attachment 8753845 [details] Bug 1272686 - Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, Marco, I fixed the last few tests that were failing, and implemented the changes you suggested. Everything seems fine on try. Do you mind taking one last look at it before I request check in?

Comment on attachment 8753845 [details] Bug 1272686 - Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, It looks great to me!

Thanks Marco. :)

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2a5a317ccfdf Change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally, r=mak

Thanks for adding those intermittents, Marco. I will take a look and see if I can update the tests to fix those issues.

Oh, I see you've already addressed one. Are you planning to do the others as well?

if we fix History.jsm, all the intermittents should be fixed by that. It depends whether my idea is correct.