58 bytes, text/x-review-board-request
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.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Depends on: 1265836
Priority: -- → P3
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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1265835
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Review commit: https://reviewboard.mozilla.org/r/57968/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57968/
Attachment #8760396 - Flags: review?(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.
Attachment #8760396 - Flags: review?(aswan) → review+
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 , not `VisitItem`s . 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.  https://developer.chrome.com/extensions/history#type-HistoryItem  https://developer.chrome.com/extensions/history#type-VisitItem
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/
Pushed by email@example.com: https://hg.mozilla.org/integration/fx-team/rev/f8bdceb55c9d Add tests for browser.history.search that use specific date ranges. r=aswan
You need to log in before you can comment on or make changes to this bug.