I found a build that was crashing on m-c this weekend in the robocop tests. I downloaded it and installed it on my tegra. This crashes and generates a .dmp crash report. The problem is the CrashReporter activity is starting up and we are moving the reports to the default profile/pending folder. ack!! This shouldn't be happening when we send in MOZ_CRASHREPORTER_NO_REPORT=1 to the environment variable. Here is what we are sending to sutagent: FIRE PROC: ' "MOZ_CRASHREPORTER_SHUTDOWN=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec.tests.testCheck3 org.mozilla.roboexample.test/org.mozilla.fennec.FennecInstrumentationTestRunner' It could be that since we are launching a process indirectly that we don't respect the environment variables.

Yeah I'm pretty sure those environment variables never make it to gecko. If you want specific things set in Gecko's environment you'll have to make sure it gets set in GeckoAppShell.setupGeckoEnvironment. Currently that code sets env variables that it picks out of the intent used to launch fennec. See also https://wiki.mozilla.org/Mobile/Fennec/Android#Arguments_and_Environment_Variables

this seems to be a specific problem with our robocop based tests since we launch them another way via am instrument. Our normal program launching sets the env vars properly as intent args

and this makes sense because we launch fennec from robocop: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java.in#67 Solution is to do what we do for the profile and push it down via robotium.config.

this patch solves the robocop.apk and mochitest part of this. I will post another patch which will fix the talos bits.

this adds the environment variables for robocop based talos tests.

Comment on attachment 669949 [details] [diff] [review] allow for environment variables to be passed to robotium.config (1.0) Review of attachment 669949 [details] [diff] [review]: ----------------------------------------------------------------- Minusing for the comma issue, but looks ok otherwise. ::: mobile/android/base/tests/BaseTest.java.in @@ +67,5 @@ > i.putExtra("args", "-no-remote -profile " + mProfile); > > + String envString = (String)config.get("envvars"); > + if (envString != "") { > + String [] envStrings = envString.split(","); nit: remove space before [] ::: testing/mochitest/runtestsremote.py @@ +395,5 @@ > + fHandle.write("rawhost=http://%s:%s/tests\n" % (options.remoteWebServer, options.httpPort)) > + > + if browserEnv: > + envstr = ','.join(['%s=%s' % (str(key), str(value)) > + for key, value in browserEnv.items()]) What if one of the values has a comma character in it? I think the options here are (1) test for a comma in the value and report an error if there is one or (2) write out each env var as a separate entry to the config file. @@ +403,5 @@ > + self._dm.removeFile(os.path.join(deviceRoot, "robotium.config")) > + self._dm.pushFile(fHandle.name, os.path.join(deviceRoot, "robotium.config")) > + os.unlink(fHandle.name) > + > + def buildBrowserEnv(self, options): Where is this called from?

Comment on attachment 669951 [details] [diff] [review] print the environment from talos to robotium.config (1.0) + envstr = ','.join(['%s=%s' % (str(key), str(value)) + for key, value in browser_config.get('env', {}).items()]) No need to do str(foo) if you're interpolating with '%s' which automagically does this. Also the indentation on the second line is a bit weird. + fHandle.write("envvars=%s\n" % envstr) Since you're using '=' to deliminate the env variables, maybe e.g. ':' would be better here? r+ with nits addressed

address the review comments.

Comment on attachment 670094 [details] [diff] [review] allow for environment variables to be passed to robotium.config (2.0) Review of attachment 670094 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with the error message fixed ::: testing/mochitest/runtestsremote.py @@ +399,5 @@ > + delim = "" > + for key, value in browserEnv.items(): > + try: > + value.index(',') > + print "Found: Error an ',' in our value, unable to process value." Found <-> Error

I know you r+'d this before, this latest change now puts the code in sync with what is r+'d for desktop and will land first thing in the morning.

Comment on attachment 670508 [details] [diff] [review] print the environment from talos to robotium.config (2.0) + try: + value.index(',') + print "Error: Found an ',' in our value, unable to process value." Um....shouldn't you check what the value is before printing the error?

the idea is that value.index will throw an exception when we don't find a ','. This means that if we find a ',' we have an error with the value and will not process it. Otherwise will be be generating much more code to handle commas inside the value such as "ENV1=a,b,c,ENV2=jhammel,ENV3=jmaher".

Comment on attachment 670508 [details] [diff] [review] print the environment from talos to robotium.config (2.0) Ah, tricky and clever. I'll go with it, though I would have coded it differently. I'm guessing that having a line per environment variable isn't an option here?

it is an option, although with the simple parser we have in robocop it would make it more complex. Let me add a comment to explain it a bit more and indicate the limitation and other options if we choose to enhance our robotium.config parsing inside of robocop.

(I've also usually used ';'s to delimit env variables in the past, but of course that is no better nor worse than ',', really)

http://hg.mozilla.org/build/talos/file/25032f0721d5 I landed the bits on inbound this morning and backed them out. Both of these need to be landed in parallel. This also means if we need to deploy talos on Aurora, we should update robocop on aurora as well.

yeah, I am no cool

Comment on attachment 671874 [details] [diff] [review] use talos variables instead of mochitest variables (1.0) I didn't verify, but this looks more correct to me. ;)