Remove PlacesTestUtils.clearHistory()

RESOLVED FIXED in Firefox 60

Status

()

Toolkit
Places
P3
normal
RESOLVED FIXED
18 days ago
9 days ago

People

(Reporter: mak, Assigned: Nathan Watson, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla60
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch][lang=js], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

18 days ago
We can directly use PlacesUtils.history.clear() now.
(Assignee)

Comment 1

18 days ago
Hey there, is this one free for the taking? I'd like to try it out.

If so - OK for this to be a single patch? (high file count, but seems test-only and pretty mechanical)
(Reporter)

Comment 2

18 days ago
yes, the only risk of a single patch is bitrotting, but I have no problems with a single patch.
Indeed, this is mostly a mechanical change, it may be useful to check your build env is well setup and get acquainted with the mozilla development process.
(Assignee)

Comment 3

15 days ago
Hey there, apologies for the slow turnaround on this.

I have a draft up on mozreview, but I'm having some difficulty running tests for it; e.g. `mach test browser/base/content/test/general/browser_contentAltClick.js` eventually fails with:

GECKO(24429) | FATAL ERROR: Non-local network connections are disabled and a connection attempt to mochi.test (198.105.244.228) was made.
GECKO(24429) | You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.

I'm not sure if this is the correct way to go about testing this, at any rate. Does this seem like the right track to be going down?
Flags: needinfo?(mak77)
(Reporter)

Comment 4

15 days ago
(In reply to Nathan Watson from comment #3)
> for it; e.g. `mach test
> browser/base/content/test/general/browser_contentAltClick.js` eventually
> fails with:
> 
> GECKO(24429) | FATAL ERROR: Non-local network connections are disabled and a
> connection attempt to mochi.test (198.105.244.228) was made.

That's strange, mochi.test is a domain defined by the test harness itself and as such it's allowed. I wonder if you have some kind of firewall/security app that intercept connections, or somehow defined a test domain locally, like in a hosts file?
Flags: needinfo?(mak77)
(Assignee)

Comment 5

14 days ago
Oh, good insight. My ISP intercepts invalid addresses and reroutes them to some "dnsrsearch.com" thing; disabling that service smoothed things out nicely. Thanks!

I'm not able to run some affected tests locally. In particular, https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/browser.ini excludes linux hosts due to flakes. I'm assuming that running against the try server would work here, which I'd need you to kick off for me?
Comment hidden (mozreview-request)
(Assignee)

Updated

14 days ago
Attachment #8948169 - Flags: review?(mak77)
(Reporter)

Updated

12 days ago
Assignee: nobody → at
Status: NEW → ASSIGNED
(Reporter)

Comment 7

12 days ago
(In reply to Nathan Watson from comment #5)
> I'm not able to run some affected tests locally. In particular,
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> newtab/browser.ini excludes linux hosts due to flakes. I'm assuming that
> running against the try server would work here, which I'd need you to kick
> off for me?

Right, no worries, it's trivial to issue a Try run from mozreview.
(Reporter)

Comment 8

12 days ago
mozreview-review
Comment on attachment 8948169 [details]
Bug 1434262 - Remove PlacesTestUtils.clearHistory()

https://reviewboard.mozilla.org/r/217724/#review223592

It looks great, you can remove a couple imports. That should not affect test results thus I started a Try run.

::: browser/base/content/test/general/browser_contentAltClick.js:18
(Diff revision 1)
>  "use strict";
>  
>  ChromeUtils.defineModuleGetter(this, "Downloads",
>                                 "resource://gre/modules/Downloads.jsm");
> -ChromeUtils.defineModuleGetter(this, "PlacesTestUtils",
> -                               "resource://testing-common/PlacesTestUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "PlacesUtils",
> +                               "resource://gre/modules/PlacesUtils.jsm");

nit: This import should not be necessary, since the head.js file in the same folder already does it and head files are included in all the tests in the same folder.

::: browser/base/content/test/general/browser_sanitizeDialog.js:30
(Diff revision 1)
>  ChromeUtils.defineModuleGetter(this, "Timer",
>                                 "resource://gre/modules/Timer.jsm");
>  ChromeUtils.defineModuleGetter(this, "PlacesTestUtils",
>                                 "resource://testing-common/PlacesTestUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "PlacesUtils",
> +                               "resource://gre/modules/PlacesUtils.jsm");

same thing here
Attachment #8948169 - Flags: review?(mak77) → review+
(Reporter)

Updated

12 days ago
Flags: needinfo?(mak77)
(Reporter)

Comment 9

12 days ago
note, it's possible bug 1167238 moved some code around, so it may need a rebase when that is marked as fixed
Comment hidden (mozreview-request)
(Assignee)

Comment 11

12 days ago
mozreview-review-reply
Comment on attachment 8948169 [details]
Bug 1434262 - Remove PlacesTestUtils.clearHistory()

https://reviewboard.mozilla.org/r/217724/#review223592

> nit: This import should not be necessary, since the head.js file in the same folder already does it and head files are included in all the tests in the same folder.

Out of curiosity, did you determine this by manual inspection, or is there some automated way to check for redundant imports?
(Reporter)

Comment 12

12 days ago
Just manual inspection
Comment hidden (mozreview-request)
(Reporter)

Comment 14

10 days ago
Comment on attachment 8948169 [details]
Bug 1434262 - Remove PlacesTestUtils.clearHistory()

I'm unbitrotting the patch for landing
Attachment #8948169 - Attachment is obsolete: true
Flags: needinfo?(mak77)
(Reporter)

Updated

10 days ago
Attachment #8948988 - Flags: review?(mak77)
(Reporter)

Comment 15

10 days ago
mozreview-review
Comment on attachment 8948988 [details]
Bug 1434262 - Remove PlacesTestUtils.clearHistory()

https://reviewboard.mozilla.org/r/218404/#review224196
Attachment #8948988 - Flags: review?(mak77) → review+

Comment 16

10 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c36d0921977e4ca2775e07d5b398f94a620e7467 -d c20f2aab0fa3: rebasing 446209:c36d0921977e "Bug 1434262 - Remove PlacesTestUtils.clearHistory() r=mak" (tip)
merging browser/base/content/test/metaTags/browser_meta_tags.js
merging browser/base/content/test/newtab/head.js
merging browser/base/content/test/sanitize/browser_sanitizeDialog.js and browser/base/content/test/general/browser_sanitizeDialog.js to browser/base/content/test/sanitize/browser_sanitizeDialog.js
merging browser/components/places/tests/browser/browser_bookmarksProperties.js
merging browser/components/places/tests/browser/browser_forgetthissite_single.js
merging browser/components/places/tests/browser/browser_library_commands.js
merging browser/components/places/tests/browser/browser_library_delete_bookmarks_in_tags.js
merging browser/components/places/tests/browser/browser_library_delete_tags.js
merging browser/components/places/tests/browser/browser_library_infoBox.js
merging browser/components/places/tests/browser/browser_library_panel_leak.js
merging browser/components/places/tests/browser/browser_paste_into_tags.js
merging browser/components/places/tests/browser/browser_sidebarpanels_click.js
merging toolkit/components/places/tests/PlacesTestUtils.jsm
merging toolkit/components/places/tests/chrome/test_favicon_annotations.xul
merging toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
merging toolkit/components/places/tests/unit/test_sync_utils.js
merging toolkit/modules/tests/xpcshell/head.js
warning: conflicts while merging browser/base/content/test/newtab/head.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/places/tests/browser/browser_library_panel_leak.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(Reporter)

Comment 17

10 days ago
hm, looks like something landed in the last hours, will have to wait the next merge.
(Assignee)

Comment 18

10 days ago
Newbie question time, apologies in advance.

by "unbitrotting", do you mean rebase and merge as necessary?

Should I be attempting to resolve the merge? I don't mind, but not sure if you're steering this from here.

btw, I saw that there were failures on try, although they might only be flakes. In general, is there a process to follow to rule out legitimate failures prior to moving forward? Or is it at our discretion after manually checking the failures in treeherder?
Flags: needinfo?(mak77)
(Reporter)

Comment 19

10 days ago
Yes, rebase and merge as necessary. Don't worry I can take care of that.
I'm waiting for a couple merges to happen and updating the patch will be trivial. It's our "fault" because in the last days we touched quite some code for refactorings.

The failures in treeherder, apart from a small test missing a change, were due to infrastructure problems, in particular a certificate had expired. Usually you can check the failures on Try by reading the failing tests and checking other trees like autoland or mozilla-central. If the other trees are closed due to infrastructure problems, it's possible those also affect Try and some of the failures could be due to that.
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)
(Reporter)

Comment 21

10 days ago
ah, I was confusing your try with a previous one. in the case of the Try run of this bug, the failures shown were intermittent failures. We have some. If you click on the orange failure, in the panel below there may be "suggestions" of what the failure is. If those suggestions match the failure, it's very likely an intermittent. In official trees (not Try) there are tree sheriffs that track those intermittent by "starring" them (if you look at autoland or central you should see failures with an asterisk). Weekly a sum up of all the failures for each test is posted by a bot to the corresponding bug. Finally, we have a dashboard called The Orange Factor that tracks the intermittent failures globally. The ones failing more than 30 times a week get prioritized.

Comment 22

9 days ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/edadd9a14891
Remove PlacesTestUtils.clearHistory() r=mak
https://hg.mozilla.org/mozilla-central/rev/edadd9a14891
Status: ASSIGNED → RESOLVED
Last Resolved: 9 days ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.