Make places be enabled by default in xpcshell-tests (aka Thunderbird Permanent orange: TEST-UNEXPECTED-FAIL | test_DownloadList.js | "Unexpected error in adding visits.")

RESOLVED FIXED in Firefox 24

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({intermittent-failure})

unspecified
mozilla25
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox24 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch The fix (obsolete) — 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 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)
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 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+
(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.
Posted patch The fix v2 (obsolete) — Splinter Review
With the selective setting.
Attachment #766612 - Attachment is obsolete: true
Attachment #767664 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dba904d5c8
Flags: in-testsuite-
Target Milestone: --- → mozilla25
Posted patch The fix v3Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/f1a090e76e61
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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?
Attachment #767733 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.