Closed
Bug 886263
Opened 11 years ago
Closed 11 years ago
Make places be enabled by default in xpcshell-tests (aka Thunderbird Permanent orange: TEST-UNEXPECTED-FAIL | test_DownloadList.js | "Unexpected error in adding visits.")
Categories
(Testing :: XPCShell Harness, defect)
Testing
XPCShell Harness
Tracking
(firefox24 fixed)
RESOLVED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox24 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 2 obsolete files)
3.58 KB,
patch
|
standard8
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Every now and again, toolkit adds some more tests that rely on places but aren't in the places sub-directory. In a lot of these cases, I don't think it makes sense to enforce module dependency. The pain point for Thunderbird is that it disables places by default. This causes various tests to fail, unless we force the pref to true. We've done this in a few tests, but with more being added, like the latest one over the weekend, I think we should just do it globally. The attached patch moves the setting of places.history.enabled to head.js. I've tested it locally and it worked fine.
Attachment #766612 -
Flags: review?(ted)
Comment 1•11 years ago
|
||
Comment on attachment 766612 [details] [diff] [review] The fix Review of attachment 766612 [details] [diff] [review]: ----------------------------------------------------------------- I don't know enough to know if this will cause any problems, I'd like to punt this to a Places peer.
Attachment #766612 -
Flags: review?(ted) → review?(mak77)
Assignee | ||
Comment 2•11 years ago
|
||
I believe in theory it shouldn't as FF has places history enabled by default, so tests which want to be turning things off should already be setting it to false in the required places.
Comment 3•11 years ago
|
||
Comment on attachment 766612 [details] [diff] [review] The fix Review of attachment 766612 [details] [diff] [review]: ----------------------------------------------------------------- I don't think there will be any problem with this, even if conceptually I preferred the old way of opting-in than this opt-out strategy. But I see that that it was adding overhead to handle the misuses, so I think it's fine to save on that time. ::: testing/xpcshell/head.js @@ +40,5 @@ > +// apps disable it. > +let (prefs = Components.classes["@mozilla.org/preferences-service;1"] > + .getService(Components.interfaces.nsIPrefBranch)) { > + prefs.setBoolPref("places.history.enabled", true); > +}; I'd appreciate if this would be #ifdef MOZ_PLACES
Attachment #766612 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #3) > I'd appreciate if this would be #ifdef MOZ_PLACES ifdef isn't available in head.js, so I keyed it off mozIAsyncHistory being included in the build.
Assignee | ||
Comment 5•11 years ago
|
||
With the selective setting.
Attachment #766612 -
Attachment is obsolete: true
Attachment #767664 -
Flags: review+
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dba904d5c8
Flags: in-testsuite-
Target Milestone: --- → mozilla25
Comment 7•11 years ago
|
||
Backed out changeset b1dba904d5c8 (bug 886263) for xpcshell orange: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e99cc9801ae1 https://tbpl.mozilla.org/php/getParsedLog.php?id=24605112&tree=Mozilla-Inbound
Assignee | ||
Comment 8•11 years ago
|
||
Patch which fixes the bustage. I'm still not sure why I couldn't reproduce this locally. However, the issue was that I hadn't taken account of the already existing code to set preferences only in parent processes and not child ones. The only change in this patch is to move the setting of the pref down a little in the function, and pick up an additional if statement for runningInParent. This passes on try server. Therefore I've relanded this as it is a minor fix to test-only code: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a090e76e61
Attachment #767664 -
Attachment is obsolete: true
Attachment #767733 -
Flags: review+
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1a090e76e61
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 767733 [details] [diff] [review] The fix v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): 836485 User impact if declined: None, test-only changes Testing completed (on m-c, etc.): Landed on m-c Risk to taking this patch (and alternatives if risky): None String or IDL/UUID changes made by this patch: None This patch fixes unit test failures for Thunderbird caused by bug 836485 which didn't set enforce that the history enabled pref was set to true. It is a test-only change, so shouldn't affect anything.
Attachment #767733 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #767733 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d5061016385
status-firefox24:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•