Closed Bug 993690 Opened 10 years ago Closed 10 years ago

Enable toolkit/webapps tests on Mac by installing apps in a directory that doesn't require admin privileges

Categories

(Firefox Graveyard :: Webapp Runtime, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
With this workaround, we can enable the tests on Mac 10.8 (see bug 981249, in particular bug 991766 comment 15).

I think there's no way to selectively disable a mochitest on a particular OS version, so I guess I'll just detect OS X 10.6 in the test itself and call |todo(false, "Doesn't work on OS X 10.6 yet, see bug XXX")|.
Attachment #8403567 - Flags: review?(myk)
Comment on attachment 8403567 [details] [diff] [review]
Patch

Review of attachment 8403567 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, works well, but please do a try run to make sure this works on build machines!

::: toolkit/webapps/MacNativeApp.js
@@ +30,5 @@
>  
>  NativeApp.prototype = {
>    __proto__: CommonNativeApp.prototype,
>  
> +  /* 

Nit: remove this trailing whitespace.

@@ +42,5 @@
> +  },
> +
> +  set rootInstallDir(aDir) {
> +    this._rootInstallDir = aDir;
> +  },

Nit: since the setter doesn't do anything special, and the getter just falls back to a default value, this could be simply a settable property with an initial value:

  rootInstallDir: LOCAL_APP_DIR,

Also, if tests are the only code that sets the value, then it would be better for it to be identified as private by naming it _rootInstallDir.  Alternately, you could make the LOCAL_APP_DIR global const into a variable, then have the tests modify it in the global scope.

::: toolkit/webapps/tests/test_hosted.xul
@@ +165,5 @@
>        yield OS.File.remove(startMenuShortcut, { ignoreAbsent: true });
>      });
>    };
>  } else if (navigator.platform.startsWith("Mac")) {
> +  installPath = OS.Path.join(OS.Constants.Path.homeDir, "Applications", "Sample hosted app.app");

I wish there was a way to test installation into both directories (on systems that support them, anyway).
Attachment #8403567 - Flags: review?(myk) → review+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> Comment on attachment 8403567 [details] [diff] [review]
> Patch
> 
> Review of attachment 8403567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, works well, but please do a try run to make sure this works on
> build machines!

I already did, it works (on 10.8). I'll re-send to try once I modify the test to make it run only on 10.8.

> @@ +42,5 @@
> > +  },
> > +
> > +  set rootInstallDir(aDir) {
> > +    this._rootInstallDir = aDir;
> > +  },
> 
> Nit: since the setter doesn't do anything special, and the getter just falls
> back to a default value, this could be simply a settable property with an
> initial value:
> 
>   rootInstallDir: LOCAL_APP_DIR,
> 
> Also, if tests are the only code that sets the value, then it would be
> better for it to be identified as private by naming it _rootInstallDir.

I'll go with this option.

> ::: toolkit/webapps/tests/test_hosted.xul
> @@ +165,5 @@
> >        yield OS.File.remove(startMenuShortcut, { ignoreAbsent: true });
> >      });
> >    };
> >  } else if (navigator.platform.startsWith("Mac")) {
> > +  installPath = OS.Path.join(OS.Constants.Path.homeDir, "Applications", "Sample hosted app.app");
> 
> I wish there was a way to test installation into both directories (on
> systems that support them, anyway).

At the beginning I was thinking about doing this, but then I thought having the same test running different things on remote and local build machines wasn't ideal. We may do it in a follow-up, though.
Attached patch Patch (obsolete) — Splinter Review
I've only added a check to skip the tests on Mac 10.6 and added (in head.js) a new function to set the dry_run pref and some constants (isLinux, isMac, isMac106, isWindows) to avoid duplicating code between tests.

I've sent the patch again to try, and it's green: https://tbpl.mozilla.org/?tree=Try&rev=7713dbaf2b15
Assignee: nobody → mar.castelluccio
Attachment #8403567 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8405339 - Flags: review?(myk)
Blocks: 981251
Comment on attachment 8405339 [details] [diff] [review]
Patch

Review of attachment 8405339 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Marco Castelluccio [:marco] from comment #3)

> At the beginning I was thinking about doing this, but then I thought having
> the same test running different things on remote and local build machines
> wasn't ideal. We may do it in a follow-up, though.

Yes, it isn't ideal!  But at least then it would be possible to test installation into /Applications, even if only on local machines.  If we do this, then we should still test installation into ~/Applications locally, so local machines run all the tests that remote build machines run, and the only difference is that local machines run some additional ones.


(In reply to Marco Castelluccio [:marco] from comment #4)
> I've only added a check to skip the tests on Mac 10.6 and added (in head.js)
> a new function to set the dry_run pref and some constants (isLinux, isMac,
> isMac106, isWindows) to avoid duplicating code between tests.

Looks good, r=myk!

::: toolkit/webapps/tests/head.js
@@ +9,5 @@
>  
> +const isLinux = navigator.platform.startsWith("Linux");
> +const isMac = navigator.platform.startsWith("Mac");
> +const isWin = navigator.platform.startsWith("Win");
> +const isMac106 = navigator.userAgent.contains("Mac OS X 10.6");

Nit: I would name these using the ALL_CAPS convention for consts (notwithstanding the C* consts, which use a different style), like in this Python code:

http://hg.mozilla.org/mozilla-central/annotate/45ba19361b97/python/psutil/test/test_psutil.py#l62

::: toolkit/webapps/tests/test_hosted.xul
@@ +210,5 @@
>    yield cleanup();
>  
> +  SimpleTest.registerCleanupFunction(function() {
> +    cleanup();
> +  });

Nit: here and elsewhere, you should be able to do simply:

  SimpleTest.registerCleanupFunction(cleanup);
Attachment #8405339 - Flags: review?(myk) → review+
Attached patch PatchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=0d2cf9b68b76
Attachment #8405339 - Attachment is obsolete: true
Attachment #8409447 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/43fa416d3ec3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.