Closed Bug 891324 Opened 6 years ago Closed 6 years ago

Make nsIXULRuntime accessible using Services.appinfo in xpcshell tests

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Currently, Services.appinfo doesn't work in xpcshell tests because it requests
the nsIXULAppInfo interface in addition to nsIXULRuntime.

A simple modification can make nsIXULAppInfo optional, allowing tests to use
Services.appinfo.OS to determine the target platform.
Attached patch The patch (obsolete) — Splinter Review
This patch includes the change in the Downloads API tests that allows them to
be consistent with the preprocessing instructions we use in the tested module.

I've also moved most of the tests for the Services.jsm module to xpcshell, as
we are using most of the services there as well, and only left the minimum
number of browser-chrome tests in place.
Attachment #772618 - Flags: review?(gavin.sharp)
Comment on attachment 772618 [details] [diff] [review]
The patch

>diff --git a/toolkit/modules/Services.jsm b/toolkit/modules/Services.jsm

> XPCOMUtils.defineLazyGetter(Services, "appinfo", function () {

>+  } catch (ex if ex instanceof Components.Exception &&
>+                 ex.result == Cr.NS_NOINTERFACE) {
>+    // We are running inside xpcshell tests, not inside an application.

nit: I suppose in theory other applications/environments in which Services.jsm could be used might hit this case, so I suggest making the comment more generic:

// Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't)

>diff --git a/toolkit/modules/tests/browser/browser_Services.js b/toolkit/modules/tests/browser/browser_Services.js
>diff --git a/toolkit/modules/tests/xpcshell/test_Services.js b/toolkit/modules/tests/xpcshell/test_Services.js

I'm not really a fan of these changes since its confusingly splits the test coverage in two across two test suites. Can we use a test-only module (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Developing_tests#JavaScript_test-only_modules) to have the tests run both in browser-chrome and xpcshell without code duplication?
Attachment #772618 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Can we use a test-only module to have the tests run both in
> browser-chrome and xpcshell without code duplication?

To implement the tests there, we'd still need the two entry point files, plus a
third one (let's say toolkit/modules/tests/common/test_Services.js), and we
should call into the module with a boolean flag to enable or disable the tests
that are specific to browser-chrome.

Then, since the assertion functions are different, we'll need to hook different
service check functions into the module, so the actual test would still be found
in the two entry point files, while only the list of tests would be in the module.

Is this what you had in mind?
(In reply to Paolo Amadini [:paolo] from comment #3)
> To implement the tests there, we'd still need the two entry point files,
> plus a
> third one (let's say toolkit/modules/tests/common/test_Services.js), and we
> should call into the module with a boolean flag to enable or disable the
> tests
> that are specific to browser-chrome.

I'm not sure I quite understand why there are so many services that seem to be specific to browser-chrome. contentPrefs, downloads, search all seem like they could be tested by xpcshell, and your patch makes appinfo testable there as well (though you need to jump through a hoop or two to test nsIXULAppInfo). Do we need browser_Services.js at all?

> Then, since the assertion functions are different, we'll need to hook
> different
> service check functions into the module, so the actual test would still be
> found
> in the two entry point files, while only the list of tests would be in the
> module.

Assuming we do want coverage in both harnesses, the module could do its own testing and just return a pass/fail + summary message to the parent for reporting. Not ideal, but less complicated.
Attached patch Updated patchSplinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> I'm not sure I quite understand why there are so many services that seem to
> be specific to browser-chrome. contentPrefs, downloads, search all seem like
> they could be tested by xpcshell, and your patch makes appinfo testable
> there as well (though you need to jump through a hoop or two to test
> nsIXULAppInfo). Do we need browser_Services.js at all?

That's right, I figured out I just had to call do_get_profile() to make those
services available to xpcshell.  I defined a mock factory to test the only
remaining case, let me know if you think we should do this rather than a
browser-chrome test (in fact, other browser-chrome tests may be already
providing enough coverage for the real service).
Attachment #772618 - Attachment is obsolete: true
Attachment #778342 - Flags: review?(gavin.sharp)
Comment on attachment 778342 [details] [diff] [review]
Updated patch

Looks good!

One typo: "Cheking"
Attachment #778342 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/db5e3bb3205d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 950001
You need to log in before you can comment on or make changes to this bug.