currently all tests which use the reftest harness fail on native fennec.

this works on my nexus S, but fails on my tegra board. What I see is when we are reading the manifest file, we do: var listURL = aURL; var channel = gIOService.newChannelFromURI(aURL); var inputStream = channel.open(); on my nexusS, the channel.isPending() = 1, but on the tegra is = 0; When I do inputStream.available(), the tegra = 0; There are still a few hacks in all of this and the big question of how to get reftest.js the same between restartless and normal extension.

so I tried using XMLHttpRequest to get the data and it failed. Also tried using http://www.google.com and this failed. doing a ping from adb shell yielded success. Also connecting to the device via SUTAgent (tcpip) works just fine. So something in birch/fennec is not recognizing the network.

(In reply to Joel Maher (:jmaher) from comment #2) > so I tried using XMLHttpRequest to get the data and it failed. Also tried > using http://www.google.com and this failed. doing a ping from adb shell > yielded success. Also connecting to the device via SUTAgent (tcpip) works > just fine. > > So something in birch/fennec is not recognizing the network. And just to troubleshoot all the obvious stuff (cause we went through some of this on IRC): * you're using the same version of birch on both nexus and tegra (confirmed this on IRC) * You're using the sutagent on both nexus and tegra (can't think why this would matter, but just to be clear, let's clarify) - (believe we confirmed this on IRC too) * what version of android is the tegra running vs. the nexus * Do you have any logcat output of the two runs (with perhaps a manifest with only one reftest loading, to keep things simple)? This is great work so far. I'll have more detailed feedback in a bit.

* YES- same version of fennec on nexus_s and tegra * YES- sutagent on both (also adb works the same) * logcat doesn't have any information at all, this failure occurs during loading of the manifest. nexus_s version: 2.3.6 tegra version: 2.2 (I believe a custom kernel from nvidia)

ack, the tegras run reftests now, but I had to do this: gWindow.setTimeout(OnRefTestLoad, 5000); it seems that on the tegra our networking layer isn't up and waiting a few seconds allows us to run the tests properly :(

My thoughts on next steps here are to get this working on try to verify we are good before butchering reftests, but try server is not building when I push from my birch branch.

ok, this works on my tegra if I do: gWindow.setTimeout(OnRefTestLoad, 10); I assume it is just putting the OnRefTestLoad() call at the bottom of the stack where the 'bring_networking_online()' call is waiting to run.

Comment on attachment 576221 [details] [diff] [review] allow reftests to work on birch (0.9) Overall, I think this is a great start. I think we should move to using a restartless reftest extension across the board, but we need to talk to dbaron about that first. The reason it was developed as a command line extension was that the original use case for reftest was as a command line addition to firefox that you would run by hand. Once we automated it and made the make targets use the python test runner the original use case has diminished over time. Now, every use of reftest goes through the python which I think is good since we can switch to a restartless extension across the board and simplify the code. The bootstrap code depends on the behavior that there is only one browser window open, which might be ok if you're fennec but not something you want to depend on for desktop. We also want to still open a XUL window for desktop, so that might be how we get around this issue there. Also, there are a set of preferences that must be made to the default preferences in the profile before the window is loaded, see http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-cmdline.js#104 for a list of those. We need to set those up in the python profile setup code if we move to using the restartless addon. Also, I really want to avoid forking reftest.js at all costs. So, I'm in favor of moving to the restartless condition across the board for both birch and xul android and desktop. This looks like a good first start, but there's still a ways to go to generalize this.

I have (maybe had) some WIP on making reftest proper a bootstrap extension. I did have trouble because doing an openWindow('reftest.xul') requires a chrome:// URI vs a file:// URI. We could get around that by opening something like chrome://browser/content and keep it as a blank window. Doing that requires us to load in all the scripts and do the hacky stuff where I get the profile directory and file the extension to do a loadSubScript and loadFrameScript off that path. I do think most of this can be done with FileUtils.jsm (for file io), moving quit.js into 2 simple lines as I do in this patch, moving reftest.js to reftest.jsm and figuring out what to do with reftest-content.js. Bug 705967 takes about 60% of these changes to reftest.js and applies them to mozilla-central. I think 1 more bug to make it restartless and then we can figure out the remaining few issues.

this patch has a problem with j1 where we crash on android-xul, otherwise it doesn't affect desktop.

updated patch with green try server run for all desktop platforms and existing android-xul. In general this adds a bootstrap.js and a reftest.jsm (a 98% copy of reftest.js). Using makefile logic, we only use the reftest.jsm and bootstrap.js when we are building on android. Why this is important is we can start running tests on nativeUI fennec by using this method. Ideally we will work on tweaking this and making reftest.js/jsm become the same so we can consider switching to a bootstrap for all reftests.

here is a link to a try server run with this patch: https://tbpl.mozilla.org/?tree=Try&rev=b0ab4d8e2f3b

Comment on attachment 578962 [details] [diff] [review] patch that will work on m-c for running tests with a --birch flag (1.0) Review of attachment 578962 [details] [diff] [review]: ----------------------------------------------------------------- So, this all looks pretty good. I reviewed the differences between reftest.jsm and reftest.js as well. I'd really like to see those merged into the same file, and I think that should be possible. Using the reftest.jsm will get us up and running on moz-central for native-android builds, and then we can work to merge them. I don't see any huge issues with the changes you had to make to get us to run in jsm mode, and I'd prefer moving to this architecture across the board. I'd like to get dbaron's eyes on those differences though just to ensure I'm not missing anything. r+ with the notes below addressed. ::: layout/tools/reftest/bootstrap.js @@ +4,5 @@ > + if (!window) return; > +} > + > +function unloadFromWindow(window) { > + if (!window) return; Why do you need this code? why not just leave the function as {} since that's essentially the behavior here on this and the loadIntoWindow function @@ +38,5 @@ > + setDefaultPrefs(); > + Components.utils.import("chrome://reftest/content/reftest.jsm"); > + > + //JMAHER: this is a hack because on the tegra we fail to do a network transaction to get the manifest > + win.setTimeout(OnRefTestLoad, 5000, win); wow, so this doesn't happen on phones? Just on tegras? ::: layout/tools/reftest/remotereftest.py @@ +112,5 @@ > defaults["pidFile"] = "" > > + self.add_option("--birch", action="store_true", dest = "birch", > + help = "test with a bootstrap addon required for birch") > + defaults["birch"] = False This makes me think that you're doing this so that we can run both XUL style reftest and birch style reftest whether this flag is set or not. I think that's a good interim solution since we have to keep supporting both for a little while more. @@ +365,4 @@ > raise devicemanager.FileError("Failed to copy extra files to device") > > def registerExtension(self, browserEnv, options, profileDir, extraArgs = ['-silent'] ): > + pass However, this change makes me wonder how it will impact the XUL reftest. I think you need a if options.birch: gate here around the pass so that in the XUL case we still do the old style extension registration rather than removing this code across the board.

updating with minor cleanup from :ctalbert's review. To reiterate, this is a temporary solution to get reftests for native android going NOW. The goal is to merge the reftest.jsm and reftest.js files together and create a single approach. I added the birch flag so we have options to tweak stuff that are specific to native fennec and won't affect XUL fennec.

Comment on attachment 580020 [details] [diff] [review] patch that will work on m-c for running tests with a --birch flag (1.1) This looks like you're forking the entirety of reftest.js. It's not ok to just copy the file -- that would mean that everybody changing reftest in the future would have to modify two files rather than one. If you need to share most but not all of the file, I'd suggest using preprocessing or something similar. Additionally, since your diff records this as a new file rather than a file copy (which is what you should have used), I can't tell what you've changed -- knowing that could be helpful in providing advice.

updated patch that uses "#if ANDROID" statements to put the mobile logic into the reftest.js file. Also hack up reftest.js a bit in the jar.mn to conditionally call it reftest.js or reftest.jsm. Overall this is looking great on tbpl. Requires the patch from bug 710877 to be applied first.

Comment on attachment 582352 [details] [diff] [review] m-c reftest.jsm and android specific bits (2.0) >+ifeq ($(OS_TARGET),Android) >+DEFINES += -DANDROID >+DIST_FILES += bootstrap.js >+endif So we already have working reftests for XUL fennec. It seems like the condition you want here is whether we're building the Android native fennec. So I think it would be better to do: ifeq ($(MOZ_BUILD_APP),mobile/android) DEFINES += -DREFTEST_JSM DIST_FILES += bootstrap.js endif and then make of the ifdefs be REFTEST_JSM (or REFTEST_BOOTSTRAP_ADDON, or something appropriate) rather than ANDROID? Or was the intent to also change the way reftests work for XUL fennec? At the top of bootstrap.js, please give a longer description of what the file is: i.e., that it's the bootstrap code for the extension in the cases where we run reftest using a restartless extension, which is for running reftests for native Android. (Or at least that's what it seems like to me -- if I'm wrong, please write the correct thing.) >+function setDefaultPrefs() { >+ //from reftest-cmdline.js Instead of saying "//from reftest-cmdline.js", you should say: // This code sets the preferences for extension-based reftest; for // command-line based reftest they are set in function XXX in // reftest-cmdline.js. These two locations should stay in sync. and then add a similar comment in reftest-cmdline.js. Probably also worth having "// FIXME: These should be in only one place.". >+ // JMAHER: this is a hack because on the tegra we fail to do a network >+ // transaction to get the manifest, but is not needed on a nexus S >+ win.setTimeout(OnRefTestLoad, 5000, win); This seems bad. Using a 5 second timeout isn't okay -- whatever requires 5 seconds most of the time will probably occasionally require more. Needing a timeout like this is a sign you're going to end up with random orange. >+ if (aReason == APP_SHUTDOWN) return; "return;" needs to be on its own line. >+/////////////////////////////// END of bootstrap code //////////////// I don't think this is particularly useful. Also, bootstrap.js uses a mix of 2-space and 4-space indent. Pick one. In reftest.js, you're introducing a bunch of new code with 2-space indent when the file uses 4-space indent. > /* Get the logfile for android tests */ > try { > logFile = prefs.getCharPref("reftest.logFile"); >+ } catch (e) {} >+ >+ try { > if (logFile) { > try { This doesn't look like it does anything useful. How could logFile be non-null if it's ever been set. That said, |logFile| should instead be |var logFile| or |let logFile|. And this whole section should use 4-space indent to match the rest of the file. remotereftest.py: Don't call the option --birch -- that's one of the twig repositories, and thus a temporary name. Maybe call it --bootstrap-addon or something similar? Again, you're sticking a bunch of 2-space indent in a 4-space indent file. runreftest.py: It doesn't look like you're doing anything with the new manifest argument to createReftestProfile. Also the |extraArgs| parameter to runTests (and the variable in remotereftest.py where it's called) doesn't really seem "extra" -- it's replacing the core reftest args. Give it a name without the word "extra"?

updated with all the review feedback. Originally I was looking to modify this for all android tests, but after the review it would be better to land this, then work on integrating things as necessary. So this patch does as recommended and only fiddles with native UI android. I would like to slowly merge these two methods into one unified solution, but that is something we can address as time goes on and might not even be necessary.

Comment on attachment 582900 [details] [diff] [review] m-c reftest.jsm and android specific bits (3.0) >diff -r e588135f62ab layout/tools/reftest/jar.mn >--- a/layout/tools/reftest/jar.mn Mon Dec 19 10:44:52 2011 -0800 >+++ b/layout/tools/reftest/jar.mn Mon Dec 19 14:40:55 2011 -0500 >@@ -1,5 +1,9 @@ > reftest.jar: > % content reftest %content/ >+ content/reftest-content.js (reftest-content.js) >+#ifdef BOOTSTRAP >+* content/reftest.jsm (reftest.js) >+#else > * content/reftest.js (reftest.js) > content/reftest-content.js (reftest-content.js) > content/reftest.xul (reftest.xul) >@@ -8,3 +12,4 @@ > % contract @mozilla.org/commandlinehandler/general-startup;1?type=reftest {32530271-8c1b-4b7d-a812-218e42c6bb23} > % category command-line-handler m-reftest @mozilla.org/commandlinehandler/general-startup;1?type=reftest > #endif >+#endif Remove the extra occurrence of "content/reftest-content.js (reftest-content.js)" that's inside the ifdef. > /* Get the logfile for android tests */ >+ var logFile = null; > try { > logFile = prefs.getCharPref("reftest.logFile"); >- if (logFile) { >- try { >- var f = FileUtils.File(logFile); >- var mfl = FileUtils.openFileOutputStream(f, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE); >- // Set to mirror to stdout as well as the file >- gDumpLog = function (msg) { >- dump(msg); >- mfl.log(msg); >- }; >- } >- catch(e) { >- // If there is a problem, just use stdout >- gDumpLog = dump; >- } >+ } catch (e) {} >+ >+ if (logFile) { >+ try { >+ var f = FileUtils.File(logFile); >+ var mfl = FileUtils.openFileOutputStream(f, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE); >+ // Set to mirror to stdout as well as the file >+ gDumpLog = function (msg) { >+#if BOOTSTRAP >+ //NOTE: on android-xul, we have a libc crash if we do a dump with %7s in the string >+ mfl.write(msg, msg.length); >+#else >+ dump(msg); >+ mfl.write(msg, msg.length); >+#endif >+ }; > } >- } catch(e) {} >+ catch(e) { >+ // If there is a problem, just use stdout >+ gDumpLog = dump; >+ } >+ } I don't see what all this moving around is doing -- how can moving the if (logFile) out of the try change anything? Leave it where it is; just add the #if BOOTSTRAP bit (though, preferably, don't duplicate the mfl.write line). (And, also, why are you changing mfl.log to mfl.write? Or was that supposed to be part of the patch where you're changing the way we use the logger?) I'm still not ok with the 5 second timeout, so marking review-.

fixed the logging stuff to not require so much change, cleaned up the duplicated file in jar.mn and got rid of the setTimeout.

Comment on attachment 583241 [details] [diff] [review] m-c reftest.jsm and android specific bits (3.1) r=dbaron

on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/419ff5d4f035