Closed Bug 1012403 Opened 5 years ago Closed 4 years ago

Some settings tests are not running

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
tracking-b2g backlog
Tracking Status
firefox41 --- fixed

People

(Reporter: dhylands, Assigned: qdot)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 6 obsolete files)

As part of investigating bug 1009746 and bug 1012214, I discovered that the settings tests listed in dom/settings/tests/chrome.ini weren't being run.

Well, dom/settings/tests/moz.build has:

if CONFIG['MOZ_B2G']:
  MOCHITEST_CHROME_MANIFESTS += ['chrome.ini']

But when I look through TBPL, I see no "oth" tests shown for any of the B2G builds.

So this means, that effectively the settings chrome tests aren't being run.
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #8424479 - Attachment is obsolete: true
Assignee: nobody → anygregor
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S2 (23may)
Attachment #8424480 - Flags: review?(bent.mozilla)
chrome.ini should just have
[test_settings.xul], not the .js file
Comment on attachment 8424480 [details] [diff] [review]
patch

I think you need a build guru to tell you how to get the testing you want.
Attachment #8424480 - Flags: review?(bent.mozilla)
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
This is also going to need to add SettingsService to the browser package manifest, otherwise the tests fail on desktop
Assignee: anygregor → kyle
Attachment #8424480 - Attachment is obsolete: true
Attachment #8435446 - Flags: review?(anygregor)
https://tbpl.mozilla.org/?tree=Try&rev=cc0b1933c80d

Yes the permissions test failed here, but it at least shows that we /are/ running the tests which is what this patch is expected to do, and it no longer fails e10s like the old patch did either.
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
I don't think we want to expose the SettingsService to all desktop builds. We have to find another solution here.
See Also: → 974100
That should just be a matter of turning the or's in the moz.build files into and's, right? MOZ_B2G and ENABLE_TESTS?
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #9)
> That should just be a matter of turning the or's in the moz.build files into
> and's, right? MOZ_B2G and ENABLE_TESTS?

According to bent we ship production firefox with ENABLE_TESTS. We have to double check with some build folks.
That'd be fine though, because we don't ship it with MOZ_B2G also, right?
So do we want to taking the Settings API out of non B2G/Mulet-desktop completely?
Flags: needinfo?(anygregor)
Oops, too broad. Do we want to taking the SettingsManager out of non B2G/Mulet-desktop completely?
Flags: needinfo?(anygregor)
Flags: needinfo?(anygregor)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #13)
> Oops, too broad. Do we want to taking the SettingsManager out of non
> B2G/Mulet-desktop completely?

The SettingsManager is only accessed via the DOM API and we don't expose it for web content.
It should be included in B2G/Mulet.
The SettingsService is accessed in our chrome code and we shouldn't use it for our Desktop build.
Flags: needinfo?(anygregor)
Target Milestone: 2.0 S4 (20june) → 2.0 S6 (18july)
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
Target Milestone: 2.1 S4 (12sep) → 2.1 S6 (10oct)
blocking-b2g: --- → backlog
Target Milestone: 2.1 S6 (10oct) → ---
blocking-b2g: backlog → ---
Fixed test, try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1bc81cff45a, though this patch also contains the required clobber to fix OS X builds.
Attachment #8435446 - Attachment is obsolete: true
Attachment #8435446 - Flags: review?(anygregor)
Pushed at a=TEST-ONLY since :gwagner is apparently too managerial to get his code reviews done in under a year now. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/756fcc79ab98
Annnnnnnnd backed out due to OS X bundle building bustage. If only all of my tree burns were this alliterative.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0acc875712
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #18)
> Annnnnnnnd backed out due to OS X bundle building bustage. If only all of my
> tree burns were this alliterative.
> 
> Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0acc875712

seems https://treeherder.mozilla.org/logviewer.html#?job_id=10649739&repo=mozilla-inbound is also a real failure from this patch (its also on the try run)
Ok, turns out that geolocation uses the existence of SettingsService to tell whether or not it should use settings so it totally breaks on desktop if I add SettingsService everywhere. Now ifdef'd it for B2G, but unfortunately that means this is no longer a=TEST-ONLY. :(
Attachment #8620817 - Attachment is obsolete: true
Try passing on all platforms other than windows due to me pulling a bad inbound: https://treeherder.mozilla.org/#/jobs?repo=try&revision=842c824eeec6

Try on windows with valid inbound:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1b6e4c13e7
Attachment #8621775 - Flags: review?(lissyx+mozillians)
Before I review and after a quick look to the patches, allow me to ask a dumb question: why? Why were those tests not being run?
Flags: needinfo?(kyle)
The reason why I hesitated to review this patch is that we were running in situations where people start using the settings service on desktop by accident. We have to make sure nobody is using the settings service on desktop.
Would a pref be enough of a guard for that?
Flags: needinfo?(kyle)
Flags: needinfo?(anygregor)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #26)
> Would a pref be enough of a guard for that?

How would the pref work?
Flags: needinfo?(anygregor)
I could imagine some flag that we have to set when we run the tests. When we don't set the flag and request the settingsService we should assert.
Oh, hmm, I guess this is created via factory and is xpcom, so we can't easily pref off creation of the service. Should we just run this test on B2G then?
Comment on attachment 8621775 [details] [diff] [review]
Patch 1 (v5) - User Timing API Implementation

Review of attachment 8621775 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/package-manifest.in
@@ +535,5 @@
>  @RESPATH@/components/messageWakeupService.manifest
>  @RESPATH@/components/SettingsManager.js
>  @RESPATH@/components/SettingsManager.manifest
> +@RESPATH@/components/SettingsService.js
> +@RESPATH@/components/SettingsService.manifest

Why do we package on browser ?
Comment on attachment 8621775 [details] [diff] [review]
Patch 1 (v5) - User Timing API Implementation

Sorry, meant to turn off review on this. We're probably just going with running this on ICS emulator only, meaning it'll be a TEST-ONLY patch.
Attachment #8621775 - Flags: review?(lissyx+mozillians)
https://hg.mozilla.org/mozilla-central/rev/c82ca63f7310
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.