Remove PlacesTestUtils.clearHistory()

RESOLVED FIXED in Firefox 60

Status

()

P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mak, Assigned: at, Mentored)

Tracking

({good-first-bug})

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

Firefox Tracking Flags

(firefox60 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
We can directly use PlacesUtils.history.clear() now.
(Assignee)

Comment 1

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
Attachment #8948169 - Flags: review?(mak77)
(Reporter)

Updated

a year ago
Assignee: nobody → at
Status: NEW → ASSIGNED
(Reporter)

Comment 7

a year 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

a year 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

a year ago
Flags: needinfo?(mak77)
(Reporter)

Comment 9

a year 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

a year 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

a year ago
Just manual inspection
Comment hidden (mozreview-request)
(Reporter)

Comment 14

a year 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

a year ago
Attachment #8948988 - Flags: review?(mak77)
(Reporter)

Comment 15

a year 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+
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

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

Comment 18

a year 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

11 months 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

11 months 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

11 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/edadd9a14891
Remove PlacesTestUtils.clearHistory() r=mak

Comment 23

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edadd9a14891
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months 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.