|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
We can directly use PlacesUtils.history.clear() now.
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)
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.
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 (22.214.171.124) 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?
(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 (126.96.36.199) 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?
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?
(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 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+
note, it's possible bug 1167238 moved some code around, so it may need a rebase when that is marked as fixed
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?
Just manual inspection
Comment on attachment 8948169 [details] Bug 1434262 - Remove PlacesTestUtils.clearHistory() I'm unbitrotting the patch for landing
Attachment #8948169 - Attachment is obsolete: true
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)
hm, looks like something landed in the last hours, will have to wait the next merge.
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?
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.
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/edadd9a14891 Remove PlacesTestUtils.clearHistory() r=mak
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.