963327 - test_path_constants.js isn't testing many things

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Gregory Szorc [:gps]]

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?

Comment 1

[Gregory Szorc [:gps]]

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Improve profile-dependent handling of OS.Constants.Path

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 3

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7fc270445d

With all review comments addressed.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eec4fc585ddd