Closed Bug 963327 Opened 6 years ago Closed 6 years ago

test_path_constants.js isn't testing many things

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
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.
Attachment #8365258 - Flags: review?(dteller)
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?
Attachment #8365258 - Flags: review?(dteller) → review+
Blocks: 964291
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7fc270445d

With all review comments addressed.
Flags: in-testsuite+
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.
https://hg.mozilla.org/mozilla-central/rev/eec4fc585ddd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.