1434262 - Remove PlacesTestUtils.clearHistory()

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Description

[Marco Bonardo [:mak]]

We can directly use PlacesUtils.history.clear() now.

Comment 1

[Nathan Watson]

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)

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Nathan Watson]

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?

Comment 4

[Marco Bonardo [:mak]]

(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?

Comment 5

[Nathan Watson]

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 6

[Nathan Watson]

Attachment: Bug 1434262 - Remove PlacesTestUtils.clearHistory()

Review commit: https://reviewboard.mozilla.org/r/217724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217724/

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Marco Bonardo [:mak]]

note, it's possible bug 1167238 moved some code around, so it may need a rebase when that is marked as fixed

Comment 10

[Nathan Watson]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217724/diff/1-2/

Comment 11

[Nathan Watson]

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?

Comment 12

[Marco Bonardo [:mak]]

Just manual inspection

Comment 13

[Marco Bonardo [:mak]]

Attachment: Bug 1434262 - Remove PlacesTestUtils.clearHistory()

Review commit: https://reviewboard.mozilla.org/r/218404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218404/

Comment 14

[Marco Bonardo [:mak]]

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

I'm unbitrotting the patch for landing

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8948988 [details]
Bug 1434262 - Remove PlacesTestUtils.clearHistory()

https://reviewboard.mozilla.org/r/218404/#review224196

Comment 16

[Mozilla Autoland]

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)

Comment 17

[Marco Bonardo [:mak]]

hm, looks like something landed in the last hours, will have to wait the next merge.

Comment 18

[Nathan Watson]

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?

Comment 19

[Marco Bonardo [:mak]]

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.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8948988 [details]
Bug 1434262 - Remove PlacesTestUtils.clearHistory()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218404/diff/1-2/

Comment 21

[Marco Bonardo [:mak]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edadd9a14891
Remove PlacesTestUtils.clearHistory() r=mak

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/edadd9a14891