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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 2 obsolete files)
37.94 KB,
patch
|
marco
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•10 years ago
|
||
> in particular bug 991766 comment 15). Bug 981249 comment 15.
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0d2cf9b68b76
Attachment #8405339 -
Attachment is obsolete: true
Attachment #8409447 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43fa416d3ec3
Flags: in-testsuite+
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43fa416d3ec3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•