1168178 - xpcshell tests aren't being run with the firefox prefs - prefs in firefox.js don't take effect

Status: REOPENED

(Testing :: XPCShell Harness, defect)

Description

[Magnus Melin [:mkmelin]]

As I've noticed, while running xcpshell tests for firefox, the firefox prefs aren't really used - which is quite confusing and certainly unexpected as what's shipped isn't necessarily tested.

I've hit this two times, see bug 1159632 comment 39 and bug 1072748 comment 10.

(in contrast, for thunderbird the thunderbird prefs ARE used)

STR:

0) back out bug 1159632 if that landed
1) set pref("browser.helperApps.deleteTempFileOnExit", true); in modules/libpref/init/all.js (like it is in browser/app/profile/firefox.js)
2) ./mach xpcshell-test toolkit/components/jsdownloads/test/unit/test_DownloadImport.js

You'll get a failure.

Comment 1

[(not currently active) Ted Mielczarek]

This is intentional, and was done as part of bug 755724. You can run tests with the browser appdir by specifying `firefox-appdir = browser` in the test manifest:
https://dxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/tests/unit/xpcshell.ini#4

This was in support of the Metro app, but it's also useful because the webapp runtime ships as a separate app.

Comment 2

[:Paolo Amadini]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> This is intentional

I'm confused. You're saying that in a normal "simple Firefox build", the environment we test with "mach xpcshell-test" is different than what we observe with "mach run", even if we call "do_get_profile"?

Comment 3

[(not currently active) Ted Mielczarek]

Yes, unless the test explicitly opts-in to using the browser environment.

Comment 4

[:Paolo Amadini]

I've just noticed I hit this bug in a patch where I conditionally enabled Async Stacks only for Desktop using "firefox.js", and xpcshell tests relying on async stacks were enabled based on the preference (bug 1158133). What I expected was that a tryserver build for all platforms would then test both cases.

In the end I had to use a complex #define in "all.js" to achieve the same effect.

Wouldn't it be better to include application-specific preferences in xpcshell? It doesn't seem very useful to test things with an environment and settings that don't exist in practice.

Comment 5

[(not currently active) Ted Mielczarek]

xpcshell tests are sort of an odd environment to begin with. They're not Firefox, they don't have a profile by default. I'm not sure that changing this would really make life that much better.

You could certainly put that pref in firefox.js, and simply have your xpcshell tests set the pref in a head.js file. Actually, that way you could even run all of your tests twice--once with the pref enabled and once with it disabled.

Comment 6

[:Paolo Amadini]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> xpcshell tests are sort of an odd environment to begin with. They're not Firefox

Yes, that's the point. Although xpcshell tests were indeed originally designed to be application-independent and platform-independent tests, I'm not sure our current usage and expectations truly reflect this. In fact we can't run them in pure isolation anyways (for example we have different file systems on different platforms).

In practice, we rightfully use xpcshell for application tests because they are the least expensive to run, and for example it wouldn't make sense to convert them to the browser-chrome suite just because the latter is designed to run with a profile by default.

It's also true, as you pointed out, that which exact environment we use doesn't change our life much, and there are easy workarounds we currently use, like the conditionals in "all.js". But I think reasoning on what xpcshell tests have become can still be good food for thought.

Comment 7

[(not currently active) Ted Mielczarek]

I am always open to re-evaluating our underlying assumptions. Since we already have support for `firefox-appdir = browser`, one thing you could try is hacking the harness to set that by default and see if anything breaks. If nothing breaks, or the breakage is minimal, it might be a feasible thing to do. Individual tests or manifests could set that to a different value if they need to test something else.

The only thing we might break would be common xpcshell tests running on other apps like Thunderbird, but that might be OK, as it'd mean other apps would have to take notice of platform expectations that Firefox sets.

Comment 8

[:Paolo Amadini]

Bug 1287660 comment 28 is an example of something that didn't have test coverage because we didn't load the Firefox preferences in xpcshell tests by default, and we didn't even realize it until we worked on the code. Ted, as a followup to comment 7, what about reopening this bug?

Comment 9

[(not currently active) Ted Mielczarek]

I'm not opposed to changing the test harness to effectively make `firefox-appdir = browser` the default. We aren't shipping the Metro browser or the webapp runtime anymore. If we ever do ship something like that again, we could just add back support for setting `firefox-appdir = somethingelse` but keep browser as the default.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

I just about ported my xpcshell test to browser-chrome until I found out about firefox-appdir.  Seems like making it the default would help avoid people tripping on a surprising environment in the Firefox test world.

Comment 11

[John Bieling (:TbSync)]

This behavior is certainly unexpected for toolkit tests, where Firefox ignores its standard prefs, but other platforms (which do not know about firefox-appdir) do not (and thus fail under certain conditions). Seeing something like firefox-appdir in toolkit tests is very surprising to me. All I can say is that I wasted half a day to find this.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to John Bieling (:TbSync) from comment #11)

This behavior is certainly unexpected for toolkit tests, where Firefox ignores its standard prefs, but other platforms (which do not know about firefox-appdir) do not (and thus fail under certain conditions). Seeing something like firefox-appdir in toolkit tests is very surprising to me. All I can say is that I wasted half a day to find this.

Add me to the list of surprised individuals.