First, the implementation from bug 958354 is only partially correct: OS.Constants.Path.userApplicationDataDir is always undefined in the xpcshell environment. So, the test doesn't actually test anything. Digging deeper, the values for UAppData, AppData, and Progs are all not defined in the test (at least on OS X). The tests for these are therefore bogus. We've historically had to monkeypatch the xpcshell environment to get it to provide AppData and UAppData. See testing/modules/AppData.jsm:makeFakeAppDir() for the preferred solution that many tests rely on. Even with that function injected, I'm still seeing OS.Constants.Path.userApplicationDataDir return undefined. Not sure why. See bug 810543 for a similar issue. It sounds like we need to throw in a do_get_profile() and then fix OS.Constants.Path to repopulate userApplicationDataDir after profile-do-change?

I'm working on a patch. Dropping a few CC's because I had originally wanted to ask a larger question about profiles and xpcshell environments. Meh.

Many properties in OS.Constants.Path are dependent on the profile being available. This patch improves their handling. Previously, we had some repeated and boilerplate code for making OS.Constants.Paths.<prop> a lazy getter. This patch eliminates the boilerplate by iterating over the properties that need to be lazy getters. AppData and UAppData are now lazy getters. test_profiledir.js has been rolled into test_path_constants.js. test_path_constants.js now emits a warning when a comparison doesn't test anything. This should help identify ineffective tests going forward.

Comment on attachment 8365258 [details] [diff] [review] Improve profile-dependent handling of OS.Constants.Path Review of attachment 8365258 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for fixing this. ::: toolkit/components/osfile/modules/osfile_async_front.jsm @@ +61,5 @@ > +// It's possible for osfile.jsm to get imported before the profile is > +// set up. In this case, some path constants aren't yet available. > +// Here, we make them lazy loaders. > + > +function constantsGetter(constProp, dirKey) { I'd prefer if you called this "pathGetter" or "lazyPathGetter". ::: toolkit/components/osfile/tests/xpcshell/test_path_constants.js @@ +25,5 @@ > if (file) { > do_check_true(!!ospath); > do_check_eq(ospath, file.path); > } else { > + dump("WARNING: " + key + " is not defined. Test may not be testing anything!\n"); Please use do_print instead of dump. @@ +51,5 @@ > + > + yield makeFakeAppDir(); > + do_check_true(!!OS.Constants.Path.userApplicationDataDir); > + > + // FUTURE: verify AppData too. In that case, can you file a followup bug?

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7fc270445d With all review comments addressed.

Ed backed this out for Windows xpcshell failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/2242c9730c09 https://tbpl.mozilla.org/php/getParsedLog.php?id=33635077&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/eec4fc585ddd The test was a little overzealous. Removed some bad tests from the test (which IMO did not warrant re-review). Verified on a Windows machine before push.