746243 - port Mochitest to Mozbase

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(not currently active) Ted Mielczarek]

Mochitest should use mozbase (mozrunner etc) instead of automation.py.

Comment 1

[cmtalbert]

Attachment: WIP mochitest changes to support using mozprofile

Going to attach some work in progress patches for this.  You will have a mochitest set of changes and a set of changes to apply against mozbase.

I've been running this by doing the following:
* checkout master from mozbase github repo
* set up virtualenv using code from mozbase (create virtualenv, run mozbase/setup_development.py)
* Apply patch to mozbase code
* Apply patch to mochitest code
* Run mochitest inside the virtualenv.

Comment 2

[cmtalbert]

Attachment: mozbase changes WIP

Comment 3

[cmtalbert]

Attachment: WIP Changes to mozbase version 2

Here is version 2 of the WIP, they need to be cleaned up, but these work.

I had to edit the regex in the proxy function and add extensions into the extensions/staged directory in order for them to be picked up by firefox.

Comment 4

[cmtalbert]

Attachment: WIP mochitest changes version 2

Comment 5

[cmtalbert]

Attachment: Mochitest changes v3

Adding new unbitrotted patches that no longer have debug output.

Comment 6

[cmtalbert]

Attachment: Mozbase changes v3

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 625627 [details] [diff] [review]
Mozbase changes v3

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

just a couple nits.

::: mozprofile/mozprofile/addons.py
@@ +232,4 @@
>              assert addon_id, 'The addon id could not be found: %s' % addon
>  
>              # copy the addon to the profile
> +            extensions_path = os.path.join(self.profile, 'extensions', 'staged')

if this is testing an older version of Firefox, staged might not be there or supported.  I believe staged is supported through release, but 1.9.2 and ESR I am not sure.

::: mozprofile/mozprofile/permissions.py
@@ +164,5 @@
> +        # TODO: So, do we care about this duplication error?
> +        #       I think no, in which case this should get nixed.
> +        #for loc in self._locations:
> +        #    if loc.isEqual(location):
> +        #        raise DuplicateLocationError(location.url())

lets remove this!

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 625625 [details] [diff] [review]
Mochitest changes v3

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

I really am not a fan of defining the default prefs twice.  Can we can that and fix the %(server)s stuff?

::: build/automation.py.in
@@ +374,5 @@
> +"extensions.installDistroAddons": False,
> +"extensions.testpilot.runStudies": False,
> +    
> +"geo.wifi.uri":
> +"http://%(server)s/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs",

don't we need to fill in %(server)s ?  This spot and a few others.

::: testing/mochitest/runtests.py
@@ +523,5 @@
>      """
>      return self.getFullPath(logFile)
>  
> +  def getDefaultPrefs(self, server, port):
> +    server = "%s:%s" % (server, port)

I don't like how you pass in a variable and then reuse it.  That could be confusing for somebody debugging if they overlook this first line.

@@ +583,5 @@
> +    prefs["extensions.getAddons.maxResults"] = 0
> +    prefs["extensions.getAddons.get.url"] = "http://%s/extensions-dummy/repositoryGetURL" % (server)
> +    prefs["extensions.getAddons.getWithPerformance.url"] = "http://%s/extensions-dummy/repositoryGetWithPerformanceURL"  % (server)
> +    prefs["extensions.getAddons.search.browseURL"] = "http://%s/extensions-dummy/repositoryBrowseURL"  % (server)
> +    prefs["extensions.getAddons.search.url"] = "http://%s/extensions-dummy/repositorySearchURL"  % (server)

these are all defined in automation.py.in, can we just define them in one place?

@@ +614,5 @@
>      return manifest
>  
>    def buildBrowserEnv(self, options):
>      """ build the environment variables for the specific test and operating system """
> +    # TODO use os.envioron.copy

s/envioron/environ/

@@ +953,5 @@
>      self.automation.installExtension(extensionPath, options.profilePath,
>                                       extensionID)
>  
> +  def getExtensionsForProfile(self, options):
> +    "Get special testing extensions, application distributed extensions, and specified on the command line ones to testing profile."

can you ensure that runtestsremote.py doesn't have its own version of installExtensionsToProfile?

Comment 9

[cmtalbert]

(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 625627 [details] [diff] [review]
> Mozbase changes v3
> 
> Review of attachment 625627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just a couple nits.
> 
> ::: mozprofile/mozprofile/addons.py
> @@ +232,4 @@
> >              assert addon_id, 'The addon id could not be found: %s' % addon
> >  
> >              # copy the addon to the profile
> > +            extensions_path = os.path.join(self.profile, 'extensions', 'staged')
> 
> if this is testing an older version of Firefox, staged might not be there or
> supported.  I believe staged is supported through release, but 1.9.2 and ESR
> I am not sure.
We don't run tests against 1.9.2 anymore and ESR is Firefox 10 at this point. So, I think we're OK here.

> 
> ::: mozprofile/mozprofile/permissions.py
> @@ +164,5 @@
> > +        # TODO: So, do we care about this duplication error?
> > +        #       I think no, in which case this should get nixed.
> > +        #for loc in self._locations:
> > +        #    if loc.isEqual(location):
> > +        #        raise DuplicateLocationError(location.url())
> 
> lets remove this!
Done.

Going to go ahead and land this in mozbase and will make a patch to apply to m-c as part of the update to the mochitest stuff.

Comment 10

[cmtalbert]

Landed changes in mozbase upstream repo: https://github.com/mozilla/mozbase/commit/e5c9cea7956ee0a1efbe10bb07e2d7f73b6e72cd

(The staged directory has been with us since version 5 at least, so I think it's a non issue.)

Comment 11

[cmtalbert]

Attachment: Fix tests for new behavior

Fixing the mozbase profile tests to accommodate the new behavior.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 645999 [details] [diff] [review]
Fix tests for new behavior

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

I really don't understand why we are making these changes.  I know how particular mochitests are and these do concern me.

::: mozprofile/tests/permissions.py
@@ +87,5 @@
>  
>          self.assertEqual(prefs[0], ('capability.principal.codebase.p1.granted',
>                                      'UniversalXPConnect'))
>          self.assertEqual(prefs[1], ('capability.principal.codebase.p1.id',
> +                                    'http://mochi.test:8888'))

I believe we have tests that use port 80, are we still able to access mochi.test/blah.html ?

@@ +109,3 @@
>          self.assertTrue(origins_decl in user_prefs[1][1])
>  
> +        proxy_check = "if (isHttp)    return 'PROXY localhost:8888';  if (isHttps || isWebSocket || isWebSocketSSL)    return 'PROXY localhost:443';"

why did this change from PROXY mochi.test to localhost?

Comment 13

[Jeff Hammel]

So this is to make up for a test failure:

F.
======================================================================
FAIL: test_nw_prefs (__main__.PermissionsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "permissions.py", line 90, in test_nw_prefs
    'http://mochi.test'))
AssertionError: ('capability.principal.codebase.p1.id', u'http://mochi.test:8888') != ('capability.principal.codebase.p1.id', 'http://mochi.test')

----------------------------------------------------------------------
Ran 2 tests in 0.575s

FAILED (failures=1)

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 645999 [details] [diff] [review]
Fix tests for new behavior

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

this is test only code, I thought it was for production.

Comment 15

[Andrew Halberstadt [:ahal]]

This breaks peptest on all platforms: https://tbpl.mozilla.org/php/getParsedLog.php?id=13969441&tree=Try

PEP DEBUG | Traceback:
Traceback (most recent call last):
  File "/home/ahal/git/peptest/peptest/runpeptests.py", line 382, in main
    peptest = applications[options.app](options)
  File "/home/ahal/git/peptest/peptest/runpeptests.py", line 61, in __init__
    proxy=enable_proxy)
  File "/home/ahal/git/mozbase/mozprofile/mozprofile/profile.py", line 75, in __init__
    prefs_js, user_js = self.permissions.network_prefs(proxy)
  File "/home/ahal/git/mozbase/mozprofile/mozprofile/permissions.py", line 270, in network_prefs
    + "://" + l.host + ":" + l.port))
TypeError: cannot concatenate 'str' and 'int' objects

Comment 16

[Andrew Halberstadt [:ahal]]

Attachment: Fix for concatenation error

This patch fixes the above error, but now I'm getting:

PEP DEBUG | Traceback:
Traceback (most recent call last):
  File "/home/ahal/git/peptest/peptest/runpeptests.py", line 382, in main
    peptest = applications[options.app](options)
  File "/home/ahal/git/peptest/peptest/runpeptests.py", line 61, in __init__
    proxy=enable_proxy)
  File "/home/ahal/git/mozbase/mozprofile/mozprofile/profile.py", line 75, in __init__
    prefs_js, user_js = self.permissions.network_prefs(proxy)
  File "/home/ahal/git/mozbase/mozprofile/mozprofile/permissions.py", line 274, in network_prefs
    user_prefs = self.pac_prefs(proxy)
  File "/home/ahal/git/mozbase/mozprofile/mozprofile/permissions.py", line 296, in pac_prefs
    webServer = proxy["webserver"]
TypeError: 'bool' object has no attribute '__getitem__'

Which I think is also related to this bug.

Comment 17

[Andrew Halberstadt [:ahal]]

Ok, so this is related to an api change. I updated some code in peptest and got it to work.. however in order to get this working I need this:
http://pastebin.mozilla.org/1724489

This seems like poor design as I now need to specify the exact same information twice (in locations.add_host, and in the proxy dict). What exactly was wrong with the old way?

Comment 18

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #16)
> Created attachment 647175 [details] [diff] [review]
> Fix for concatenation error
> 
> This patch fixes the above error, but now I'm getting:

Should we get this reviewed and checked in?

Comment 19

[Andrew Halberstadt [:ahal]]

(In reply to Jeff Hammel [:jhammel] from comment #18)
> Should we get this reviewed and checked in?

Yeah, I can check it in. I just wanted to double check that we actually want an API that lends itself to the code in the pastebin. It seems somewhat silly to force people to specify the host and port twice in the same function call.

Comment 20

[Jeff Hammel]

FWIW, I would prefer not to double-specify information unless there is some utility in doing so.  :ctalbert, can you comment on this when you have the chance?

That said, the string concatenation patch doesn't have anything to do with the API and I think we should take it.

Comment 21

[Andrew Halberstadt [:ahal]]

Pushed strcat fix to master: https://github.com/mozilla/mozbase/commit/37da8e2fa7ec75268178d842a0d61652d987cba3

Comment 22

[Andrew Halberstadt [:ahal]]

Attachment: Mozbase changes v3.1

So I think there is a problem with special casing sslport. A port is a port is a port.

This patch addresses that, but it might break some of the work you did in the mochitest harness Clint. So let me know if I should revise this at all (or throw it out completely).

Another approach might be to have proxy as a dict like before, but then completely get rid of the notion of primary locations.

Comment 23

[Jeff Hammel]

Comment on attachment 647957 [details] [diff] [review]
Mozbase changes v3.1

Looks great, Andrew. Thanks

Comment 24

[cmtalbert]

Comment on attachment 647957 [details] [diff] [review]
Mozbase changes v3.1

I'm not certain how this will proxy https (ssl) requests. We need proxy lines to proxy them as I understood it. So, I'm not sure this is going to work with mochitest which needs SSL support.

Have you tried it with mochitest?

This will pass the mozbase tests fine because we don't have tests that test the proxying over SSL. So we really need to run it with mochitest to understand if it works.

Comment 25

[Andrew Halberstadt [:ahal]]

Comment on attachment 647957 [details] [diff] [review]
Mozbase changes v3.1

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

::: mozprofile/mozprofile/permissions.py
@@ +186,5 @@
> +                default_ports = {'http': '80',
> +                                 'https': '443',
> +                                 'ws': '443',
> +                                 'wss': '443'}
> +                port = default_ports.get(scheme, '80')

This should be passed along to the mochitest harness, so I think it will still work. I'll push a full try run and if there are problems I'll fix it.

Comment 26

[Andrew Halberstadt [:ahal]]

The try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=d0120042a387

The peptest bustage is due to a different recent change in mozbase and I will update the peptest harness at the same time as syncinc mozbase. With that said, I'm going to push this to master. I'm 95% confident this won't break anything, but I'll keep an eye out for strange happenings.

Comment 27

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozilla/mozbase/commit/ddab19ab677652d23023c6f11752c86163037da3

Comment 28

[Jeff Hammel]

Doesn't this require the actual mochitest changes?  Shouldn't this be reopened?

Comment 29

[Andrew Halberstadt [:ahal]]

My mistake, I thought I had re-opened it when I found the string concatenation error. Good catch :p

Comment 30

[cmtalbert]

I'm not working on this right now, resetting assignee to reflect that.

Comment 31

[Jeff Hammel]

Apropos initial refactor plans, its been decided that the strategy we're going to use is to refactor mochitest (i.e. http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py ) to use mozbase modules, in contrast to rewriting automation.py.in .  An advantage of this method and AIUI the reason this is the chosen path is that since many harnesses (etc) use automation.py(.in) that refactoring runtests.py allows the refactor to take place without disruption to these other pieces of software.  As needed/applicable, refactoring of automation.py.in may be done as support or in parallel, but that's not where we are putting the weight of this task's execution. Ultimately, still, we want automation.py.in to be gone. This as a part of the overall strategy of migrating mozilla-central software to mozbase should be documented.

Comment 32

[Andrew Halberstadt [:ahal]]

Just for the record, an incarnation of this patch already landed for B2G mochitests in bug 835930 (+ dependencies). The patch over there might be slightly less bitrotted than the one in this bug.

Comment 33

[Jeff Hammel]

There's some duplicate code in MochitestServer.start and WebsocketServer.start:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#87
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#148

I'm not going to conflate this bug with this particular cleanup, but IMHO this should be fixed.  Will file a followup

Comment 34

[Jeff Hammel]

Attachment: wip : bug-746243.diff

Very sadly, this is a WIP: tests do run, but the output handling is not yet worked out.

I updated the following files, deprecating `automation` references:
runtest.py, mochitest_options.py, mach_commands.py .

`automation.Process` was replaced with `mozprocess.ProcessHandler`.
Several functions were rewritten or refactored in the process.

One function utilized from automation.py that could not be cleanly
eliminated is Automation.environment():

http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#474

Utilized in:
- http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#64
- http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#492

For this bug, I think it makes sense to migrate `environment` to
runtests.py . But we should find a better place to put it longer term,
perhaps. should this go somewhere centralized? (Perhaps in
automationutils.py for now?)

The PID checking logic for MochitestServer and WebsocketServer appeared unneeded:

  http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#89

`mochitest_options.py` uses automation.py for option defaults:
http://hg.mozilla.org/mozilla-central/file/416075f77249/testing/mochitest/mochitest_options.py#l332

However, there is in fact one key in common, environment:

>>> opts
['a11y', 'app', 'autorun', 'browserArgs', 'browserChrome', 'certPath',
'chrome', 'chunkByDir', 'closeWhenDone', 'consoleLevel', 'debugger',
'debuggerArgs', 'debuggerInteractive', 'environment',
'extensionsToExclude', 'extensionsToInstall', 'extraPrefs',
'extraProfileFiles', 'failureFile', 'fatalAssertions', 'fileLevel',
'httpdPath', 'immersiveMode', 'ipcplugins', 'leakThreshold',
'logFile', 'manifestFile', 'profilePath', 'repeat', 'runOnlyTests',
'runSlower', 'runUntilFailure', 'shuffle', 'symbolsPath',
'testManifest', 'testPath', 'testingModulesDir', 'thisChunk',
'timeout', 'totalChunks', 'utilityPath', 'vmwareRecording',
'webapprtChrome', 'webapprtContent', 'xrePath']
>>> all
['CERTS_SRC_DIR', 'DEFAULT_APP', 'DEFAULT_TIMEOUT', 'DIST_BIN',
'IS_DEBUG_BUILD', 'IS_MAC', 'IS_TEST_BUILD', 'IS_WIN32', 'Process',
'UNIXISH', 'addCommonOptions', 'environment', 'initializeProfile',
'log', 'runApp']
>>> set(all).intersection(opts)
set(['environment'])

This is a function in `Automation`, though the default actually
(correctly) resolves to `[]`, which is not the return value of
`Automation().environment()`.  This is because `addCommonOptions` in
fact ignores the `defaults` parameter entirely except for
'SYMBOLS_PATH'. (Incidentally, mutable parameter defaults are bad.)
This code was eliminated.  Also, herein `defaults` was not actually
even used. Killed.

mochitest_options also utilized the server location defaults from
automation.py:
http://hg.mozilla.org/mozilla-central/file/416075f77249/testing/mochitest/mochitest_options.py#l388
The server I replaced with hardcoded '127.0.0.1' and the ports I took
from `mozprofile.permissions.DEFAULT_PORTS`. Currently, these options
are being overridden in `verifyOptions`. I don't know why.

`Automation.buildCommandLine` is redundant with mozrunner and was not
ported. However, mozrunner is missing the `-bin` fix --
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#763
-- and the `CYGWIN` fix --
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#778
. These may be considered for porting -> mozrunner.

In general, there are a few places where the "normal" OO assumption
that one may call methods in some arbitrary order breaks down.

The debugger section in runApp is incomplete.  I will fix shortly.

Several mozbase improvements were suggested by this work:

- [mozprocess] ProcessHandler.run() should return the PID
  https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L535

- [mozprocess] self.pid should return 'None' before process start
  and after process finish

- [mozprocess] ProcessHandler should add .poll(), .terminate() (and/or .send_signal())
  http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#114
  http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#116
  Note that reimplementing ProcessHandler as a subclass of
  subprocess.Popen -- bug 819547 -- takes care of the above

- [mozprocess] call convenience method I added to runttests.py should
  be upstreamed:
  https://bugzilla.mozilla.org/show_bug.cgi?id=791383

- [mozrunner] `Automation.buildCommandLine` is redundant with
  mozrunner and was not ported. However, the isMac -bin logic was not
  ported:
  http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#763
  Should this be included in mozrunner?

- [mozrunner] Runner.wait() should return status code

- [mozrunner] isMac automation.py.in code supports '-bin'; should mozrunner?
  http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#763

- [mozrunner] Runner shoul front-end stdout, stderr (, etc)

- [mozprofile] AddonManager should only cleanup on __del__ optionally:
  https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L266
"""
Currently, AddonManager always restores the profile to the state before its
instantiation. Instead, a parameter should be added to __init__ to
control this functionality.
"""

- make extensionID a parameter to install_from_path
  https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L169


- move certificate stuff -> mozprofile
  http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#408

- mozinfo isUnix should come from a set of unixy platforms
  `unixy = set(['linux', 'bsd', ...])`

- [mozinfo] mozinfo should have this code:
180     while path != os.path.expanduser('~'):
181         if path in dirs:
182             break
183         dirs.append(path)
184         path = os.path.split(path)[0]
185
186     mozinfo.find_and_update_from_json(*dirs)


Following the landing of whatever subset of the above is deemed
necessary for this bug, mozbase packages should be mirrored to m-c:
mozprofile, mozprocess, mozrunner, ...

Unrelated directly to this bug, several items of future work were
noted in the process of editing.  These could be cleaned up as
priority dictates:

- WebsocketServer : to inherit from MochitestServer?

- ASAN_OPTIONS configuration -> its own function

- @localhost decorator:
  def localhost(func):
      """return immediately if not 127.0.0.1"""
      address = 127.0.0.1
      def call_if_localhost(*args, **kwargs):
          if kwargs['options'].webserver != address:
             return
          return func(*args, **kwargs)
      return call_if_localhost
  # really, options shouldn't be passed...just the address

- shouldn't pass options around, just the data we need

- indentation: not only is it bad (meaning: 2 spaces) it is
  inconsistent:

- static port: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#283

- MochiTest buildProfile returns the manifest but sticks the profile
  object at self.profile.  This relies on side-effects and isn't stateful

- RFE: way to keep profile post testing

- remove jsonString

- more idiomatic logging (duh)

- MochiTest.__init__ : os.chdir(SCRIPT_DIR) ? why?

- shebang

- (packaging)

- cast to str() : why windows-specific?
  http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#491

- port ZipFileReader to mozfile:
  http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#55

- first serverlocation is popped in `fillcertificatedb`; why?
  http://hg.mozilla.org/mozilla-central/file/21b5af569ca2/build/automation.py.in#l419

-
  http://hg.mozilla.org/mozilla-central/file/21b5af569ca2/build/automation.py.in#l420
  is not only very inefficient, it is also not DRY for all the wrong
  reasons (that is, "because it isn't")

Comment 35

[(not currently active) Ted Mielczarek]

(In reply to Jeff Hammel [:jhammel] from comment #34)
> One function utilized from automation.py that could not be cleanly
> eliminated is Automation.environment():

We don't need all of automation.environment() for running a browser (the LD_LIBRARY_PATH bits are unnecessary nowadays), but we do need the rest of it, and we need the PATH bits for running xpcshell/certutil/etc, so we'll need to keep that somewhere, I suppose. I talked about putting the LD_LIBRARY_PATH bits into a helper function because runcppunittests.py uses the same thing.

> For this bug, I think it makes sense to migrate `environment` to
> runtests.py . But we should find a better place to put it longer term,
> perhaps. should this go somewhere centralized? (Perhaps in
> automationutils.py for now?)

automationutils.py is fine as a dumping ground for "stuff that's common to in-tree harnesses but we don't feel like putting in mozbase", even if it's just transitional.

> mochitest_options also utilized the server location defaults from
> automation.py:
> http://hg.mozilla.org/mozilla-central/file/416075f77249/testing/mochitest/
> mochitest_options.py#l388
> The server I replaced with hardcoded '127.0.0.1' and the ports I took
> from `mozprofile.permissions.DEFAULT_PORTS`. Currently, these options
> are being overridden in `verifyOptions`. I don't know why.

This is probably just some variant of "it's hard to provide defaults which kind of depend on other defaults or runtime values". I don't think the current structure matters much.

> `Automation.buildCommandLine` is redundant with mozrunner and was not
> ported. However, mozrunner is missing the `-bin` fix --
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> in?rev=a3e34b989ebe#763
> -- and the `CYGWIN` fix --
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> in?rev=a3e34b989ebe#778
> . These may be considered for porting -> mozrunner.

These are historical cruft and can be dropped.

> - [mozrunner] `Automation.buildCommandLine` is redundant with
>   mozrunner and was not ported. However, the isMac -bin logic was not
>   ported:
>  
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> in?rev=a3e34b989ebe#763
>   Should this be included in mozrunner?

Per previous, this can be dropped.

> - move certificate stuff -> mozprofile
>   http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#408

This stuff is a PITA because it relies on the NSS commandline utils, but putting it in mozprofile makes sense. The ssltunnel-related stuff could just be pulled out into a utility method in automationutils.py, it doesn't have anything to do with the profile (it's just jammed in there for convenience).

> 
> - mozinfo isUnix should come from a set of unixy platforms
>   `unixy = set(['linux', 'bsd', ...])`
> 
> - [mozinfo] mozinfo should have this code:
> 180     while path != os.path.expanduser('~'):
> 181         if path in dirs:
> 182             break
> 183         dirs.append(path)
> 184         path = os.path.split(path)[0]
> 185
> 186     mozinfo.find_and_update_from_json(*dirs)

Not sure why this is necessary? mozinfo.find_and_update_from_json(os.path.dirname(__file__)) should be all we ever need.

> - WebsocketServer : to inherit from MochitestServer?

MochitestServer is mostly overkill. Ideally in the long run we'd replace it with mozhttpd, but that has other issues.

> - shouldn't pass options around, just the data we need

Agree in theory, but this gets ugly pretty quickly.

> - indentation: not only is it bad (meaning: 2 spaces) it is
>   inconsistent:

I'd r+ a one-time reindent-to-4-spaces-consistently patch for all of the in-tree code whenever anyone feels like doing that. Doing it on top of your pathces might save you some headaches, but if you want to just rip that band-aid off now we could do it anytime.

> - static port:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.
> py#283

This sucks but fixing it will involve changing a lot of tests, so I would punt this to the future.

> - MochiTest.__init__ : os.chdir(SCRIPT_DIR) ? why?

Historical reasons, I suspect. We could almost certainly fix that without breaking things.

> - (packaging)

?
 
> - cast to str() : why windows-specific?

Unicode environment bug in Python 2.7.2, almost certainly.

> - port ZipFileReader to mozfile:

No, just rip this out and replace it with zipfile since we require 2.7 now (this was a < 2.7 compat shim).

> - first serverlocation is popped in `fillcertificatedb`; why?

This was filed as bug 879740. Short answer: probably doesn't matter anymore.

Comment 36

[Jeff Hammel]

+ # XXX: del the __del__ 
+ # -AddonManager should only cleanup on __del__ optionally: 
+ # https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/addons.py#L266 
+ del addons.__del__

see bug 911218

Comment 37

[Jeff Hammel]

Thanks for the feedback, Ted.  Notes below.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> (In reply to Jeff Hammel [:jhammel] from comment #34)
> > One function utilized from automation.py that could not be cleanly
> > eliminated is Automation.environment():
> 
> We don't need all of automation.environment() for running a browser (the
> LD_LIBRARY_PATH bits are unnecessary nowadays), but we do need the rest of
> it, and we need the PATH bits for running xpcshell/certutil/etc, so we'll
> need to keep that somewhere, I suppose. I talked about putting the
> LD_LIBRARY_PATH bits into a helper function because runcppunittests.py uses
> the same thing.
> 
> > For this bug, I think it makes sense to migrate `environment` to
> > runtests.py . But we should find a better place to put it longer term,
> > perhaps. should this go somewhere centralized? (Perhaps in
> > automationutils.py for now?)
> 
> automationutils.py is fine as a dumping ground for "stuff that's common to
> in-tree harnesses but we don't feel like putting in mozbase", even if it's
> just transitional.

I will migrate to automationutils.py and remove the LD_LIBRARY_PATH bits.


> > `Automation.buildCommandLine` is redundant with mozrunner and was not
> > ported. However, mozrunner is missing the `-bin` fix --
> > http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> > in?rev=a3e34b989ebe#763
> > -- and the `CYGWIN` fix --
> > http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> > in?rev=a3e34b989ebe#778
> > . These may be considered for porting -> mozrunner.
> 
> These are historical cruft and can be dropped.

Even better. Dropping.

> > - [mozrunner] `Automation.buildCommandLine` is redundant with
> >   mozrunner and was not ported. However, the isMac -bin logic was not
> >   ported:
> >  
> > http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> > in?rev=a3e34b989ebe#763
> >   Should this be included in mozrunner?
> 
> Per previous, this can be dropped.

Likewise.

> > - move certificate stuff -> mozprofile
> >   http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#408
> 
> This stuff is a PITA because it relies on the NSS commandline utils, but
> putting it in mozprofile makes sense. The ssltunnel-related stuff could just
> be pulled out into a utility method in automationutils.py, it doesn't have
> anything to do with the profile (it's just jammed in there for convenience).

Cool.  I'll figure out what to do here and file the appropriate bugs.  probably better as a follow-up(s) than as part of this. 
 
> > - mozinfo isUnix should come from a set of unixy platforms
> >   `unixy = set(['linux', 'bsd', ...])`
> > 
> > - [mozinfo] mozinfo should have this code:
> > 180     while path != os.path.expanduser('~'):
> > 181         if path in dirs:
> > 182             break
> > 183         dirs.append(path)
> > 184         path = os.path.split(path)[0]
> > 185
> > 186     mozinfo.find_and_update_from_json(*dirs)
> 
> Not sure why this is necessary?
> mozinfo.find_and_update_from_json(os.path.dirname(__file__)) should be all
> we ever need.

You mean the dir climbing stuff? Damned if I know, I'm cargo culting.  Added by ahal in https://hg.mozilla.org/mozilla-central/diff/e5c726f690f8/testing/mochitest/runtests.py . https://bugzilla.mozilla.org/show_bug.cgi?id=903034 .

If we want to keep it, easy enough to port to mozinfo.

> > - WebsocketServer : to inherit from MochitestServer?
> 
> MochitestServer is mostly overkill. Ideally in the long run we'd replace it
> with mozhttpd, but that has other issues.
> 
> > - shouldn't pass options around, just the data we need
> 
> Agree in theory, but this gets ugly pretty quickly.

I'm sure we could figure out some good pattern if it was worth it.  However, not a priority at the moment.

> > - indentation: not only is it bad (meaning: 2 spaces) it is
> >   inconsistent:
> 
> I'd r+ a one-time reindent-to-4-spaces-consistently patch for all of the
> in-tree code whenever anyone feels like doing that. Doing it on top of your
> pathces might save you some headaches, but if you want to just rip that
> band-aid off now we could do it anytime.

The desire is there; the time...not so much :( Ignoring the high increase in bitrottability of such patches.

> > - static port:
> > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.
> > py#283
> 
> This sucks but fixing it will involve changing a lot of tests, so I would
> punt this to the future.

Punting.

> > - MochiTest.__init__ : os.chdir(SCRIPT_DIR) ? why?
> 
> Historical reasons, I suspect. We could almost certainly fix that without
> breaking things.

Is there a bug or should I file one?

> > - (packaging)
> 
> ?

Eh, out of scope here.  Was going to go into more detail, but I'll punt.

> > - cast to str() : why windows-specific?
> 
> Unicode environment bug in Python 2.7.2, almost certainly.

My guess as well; will note in comment.

> > - port ZipFileReader to mozfile:
> 
> No, just rip this out and replace it with zipfile since we require 2.7 now
> (this was a < 2.7 compat shim).

Even better.

> > - first serverlocation is popped in `fillcertificatedb`; why?
> 
> This was filed as bug 879740. Short answer: probably doesn't matter anymore.

Cool; will note and possibly delete if time.

Comment 38

[(not currently active) Ted Mielczarek]

(In reply to Jeff Hammel [:jhammel] from comment #37)
> > > - MochiTest.__init__ : os.chdir(SCRIPT_DIR) ? why?
> > 
> > Historical reasons, I suspect. We could almost certainly fix that without
> > breaking things.
> 
> Is there a bug or should I file one?

http://farm6.staticflickr.com/5518/9577611301_c099673cac.jpg

Comment 39

[Jeff Hammel]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #38)
> (In reply to Jeff Hammel [:jhammel] from comment #37)
> > > > - MochiTest.__init__ : os.chdir(SCRIPT_DIR) ? why?
> > > 
> > > Historical reasons, I suspect. We could almost certainly fix that without
> > > breaking things.
> > 
> > Is there a bug or should I file one?
> 
> http://farm6.staticflickr.com/5518/9577611301_c099673cac.jpg

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=912243

Comment 40

[Jeff Hammel]

Noticed a long-standing cleanup bug in mozprofile in my troubleshooting here: bug 912338 . It is not necessarily a hard blocker, but should be a quick fix and mozprofile needs updating on m-c anyway.

Comment 41

[Jeff Hammel]

I was hoping to have a try run or two in by now, but am still debugging some problems locally.

Sometime between attachment 797473 [details] [diff] [review] and now I've introduced a bug where
the websocket tests break :( The harness dies at
/tests/content/base/test/websocket_hybi/test_send-blob.html :

62137 INFO TEST-PASS | /tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html | should be equal to 'arraybuffer'
62138 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html | lengths not equal - got 0, expected 3
62139 INFO TEST-PASS | /tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html | Checking message #0.
62140 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html | Should receive an arraybuffer!
62141 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html | uncaught exception - Error: invalid arguments at http://mochi.test:8888/tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html:80
JavaScript error: http://mochi.test:8888/tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html, line 80: invalid arguments
62142 INFO TEST-END | /tests/content/base/test/websocket_hybi/test_receive-arraybuffer.html | finished in 172ms
62143 INFO TEST-START | /tests/content/base/test/websocket_hybi/test_receive-blob.html
62144 INFO TEST-PASS | /tests/content/base/test/websocket_hybi/test_receive-blob.html | should be 'blob'
62145 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_receive-blob.html | lengths not same - got 0, expected 3
62146 INFO TEST-PASS | /tests/content/base/test/websocket_hybi/test_receive-blob.html | Checking message #0.
62147 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_receive-blob.html | We should be receiving a Blob
62148 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_receive-blob.html | uncaught exception - TypeError: Argument 1 of FileReader.readAsArrayBuffer is not an object. at http://mochi.test:8888/tests/content/base/test/websocket_hybi/test_receive-blob.html:75
JavaScript error: http://mochi.test:8888/tests/content/base/test/websocket_hybi/test_receive-blob.html, line 75: Argument 1 of FileReader.readAsArrayBuffer is not an object.
62149 INFO TEST-END | /tests/content/base/test/websocket_hybi/test_receive-blob.html | finished in 14074ms
62150 INFO TEST-START | /tests/content/base/test/websocket_hybi/test_send-arraybuffer.html
62151 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_send-arraybuffer.html | onerror should not have been called
62152 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/websocket_hybi/test_send-arraybuffer.html | should have closed cleanly
62153 INFO TEST-END | /tests/content/base/test/websocket_hybi/test_send-arraybuffer.html | finished in 21116ms
62154 INFO TEST-START | /tests/content/base/test/websocket_hybi/test_send-blob.html
Mochitest INFO | INFO | runtests.py | Application ran for: 0:05:31.065137
 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400 not CONNECT request but GET could not read the connect request, closing connection with 400
 5:32.59 WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
Mochitest INFO | runtests.py | Running tests: end.


I've been debugging this locally as well as other issues but so far
the cause eludes me.

I've also noticed that I broke preference interpolation:

diff pre/view-profile.txt now/view-profile.txt
1c1
< [Path]: /home/jhammel/mozilla/mozbase/bug-746243/pre/profile
---
> [Path]: /tmp/tmpbVlCJA
4c4
< /home/jhammel/mozilla/mozbase/bug-746243/pre/profile
---
> /tmp/tmpbVlCJA
97,99c97,99
< browser.safebrowsing.gethashURL: http://127.0.0.1:8888/safebrowsing-dummy/gethash
< browser.safebrowsing.keyURL: http://127.0.0.1:8888/safebrowsing-dummy/newkey
< browser.safebrowsing.updateURL: http://127.0.0.1:8888/safebrowsing-dummy/update
---
> browser.safebrowsing.gethashURL: http://%(server)s/safebrowsing-dummy/gethash
> browser.safebrowsing.keyURL: http://%(server)s/safebrowsing-dummy/newkey
> browser.safebrowsing.updateURL: http://%(server)s/safebrowsing-dummy/update
105c105
< datareporting.healthreport.documentServerURI: http://127.0.0.1:8888/healthreport/
---
> datareporting.healthreport.documentServerURI: http://%(server)s/healthreport/
130c130
< extensions.blocklist.url: http://127.0.0.1:8888/extensions-dummy/blocklistURL
---
> extensions.blocklist.url: http://%(server)s/extensions-dummy/blocklistURL
133,134c133,134
< extensions.getAddons.get.url: http://127.0.0.1:8888/extensions-dummy/repositoryGetURL
< extensions.getAddons.getWithPerformance.url: http://127.0.0.1:8888/extensions-dummy/repositoryGetWithPerformanceURL
---
> extensions.getAddons.get.url: http://%(server)s/extensions-dummy/repositoryGetURL
> extensions.getAddons.getWithPerformance.url: http://%(server)s/extensions-dummy/repositoryGetWithPerformanceURL
136,138c136,138
< extensions.getAddons.search.browseURL: http://127.0.0.1:8888/extensions-dummy/repositoryBrowseURL
< extensions.getAddons.search.url: http://127.0.0.1:8888/extensions-dummy/repositorySearchURL
< extensions.hotfix.url: http://127.0.0.1:8888/extensions-dummy/hotfixURL
---
> extensions.getAddons.search.browseURL: http://%(server)s/extensions-dummy/repositoryBrowseURL
> extensions.getAddons.search.url: http://%(server)s/extensions-dummy/repositorySearchURL
> extensions.hotfix.url: http://%(server)s/extensions-dummy/hotfixURL
140c140
< extensions.update.background.url: http://127.0.0.1:8888/extensions-dummy/updateBackgroundURL
---
> extensions.update.background.url: http://%(server)s/extensions-dummy/updateBackgroundURL
142,143c142,143
< extensions.update.url: http://127.0.0.1:8888/extensions-dummy/updateURL
< extensions.webservice.discoverURL: http://127.0.0.1:8888/extensions-dummy/discoveryURL
---
> extensions.update.url: http://%(server)s/extensions-dummy/updateURL
> extensions.webservice.discoverURL: http://%(server)s/extensions-dummy/discoveryURL
149c149
< geo.wifi.uri: http://127.0.0.1:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs
---
> geo.wifi.uri: http://%(server)s/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs
171c171
< plugins.update.url: http://127.0.0.1:8888/plugins-dummy/updateCheckURL
---
> plugins.update.url: http://%(server)s/plugins-dummy/updateCheckURL

This needs to be fixed.

I've been fixing blocking mozbase issues in the background as well as
cleaning up the patch, but trying to isolate the websocket issue as
well as a few other issues already present in Friday's patch has been
taking the majority of my time and attention.

I'll upload a new WIP shortly.

Comment 42

[Jeff Hammel]

> I've also noticed that I broke preference interpolation:

This is just me forgetting to port the code over:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#361

Fixed.  The preferences are now identical.  However, the code is...questionable:

    interpolation = {"server": "%s:%s" % (self.webServer, self.httpPort)}
    prefs = json.loads(json.dumps(prefs) % interpolation)
    for pref in prefs:
        prefs[pref] = Preferences.cast(prefs[pref])

We dump and load the preferences to get string interpolation which is probably bad.  More mysteriously, we call Preferences.cast() on each preference....which should be completely redundant and can only do bad things (that is, eliminate string quotes which were intended; unlikely and does not happen in production, but still):

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/prefs.py#L57

Instead, we should add an `interpolate` function in mozprofile Preferences and kill all this code.

Comment 43

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #42)
> > I've also noticed that I broke preference interpolation:
> 
> This is just me forgetting to port the code over:
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> in?rev=a3e34b989ebe#361
> 
> Fixed.  The preferences are now identical.  However, the code
> is...questionable:
> 
>     interpolation = {"server": "%s:%s" % (self.webServer, self.httpPort)}
>     prefs = json.loads(json.dumps(prefs) % interpolation)
>     for pref in prefs:
>         prefs[pref] = Preferences.cast(prefs[pref])
> 
> We dump and load the preferences to get string interpolation which is
> probably bad.  More mysteriously, we call Preferences.cast() on each
> preference....which should be completely redundant and can only do bad
> things (that is, eliminate string quotes which were intended; unlikely and
> does not happen in production, but still):
> 
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/prefs.
> py#L57
> 
> Instead, we should add an `interpolate` function in mozprofile Preferences
> and kill all this code.

Filed bug 913152

Comment 44

[Jeff Hammel]

> Sometime between attachment 797473 [details] [diff] [review] [diff] [review] and
> now I've introduced a bug where the websocket tests break :( The harness dies at
> /tests/content/base/test/websocket_hybi/test_send-blob.html 

So part of the problem is the overloading of the websocket port in
mozprofile.permissions.
websocket port is messed up in mozprofile; it is (wrongly)
set to the SSL proxy setting; see:
- https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/permissions.py#L27
vs
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#52
- https://github.com/mozilla/mozbase/commit/43f9510e3d58bfed32790c82a57edac5f928474d

        diff pre/profile/ssltunnel.cfg now/profile/ssltunnel.cfg
        2c2
        < certdbdir:/home/jhammel/mozilla/src/mozilla-try/build/pgo/certs
        ---
        > certdbdir:/home/jhammel/mozilla/src/mozilla-central/build/pgo/certs
        4c4
        < websocketserver:127.0.0.1:9988
        ---
        > websocketserver:127.0.0.1:4443

Will file.

I've fixed this by using the old default in mochitest_options.py, but
errors persist.  Will continue to troubleshoot.

Comment 45

[Jeff Hammel]

Attachment: bug-746243 WIP2

Still outstanding are a few output processors for the firefox process stdout, checking for zombies, and minor fixes needed in mozbase code as well as the mirroring of the appropriate mozbase packages themselves.

Comment 46

[Jeff Hammel]

Attachment: bug-746243 wip

This is still a wip, but it does successfully run to completion.  What is left to do is cleanup (the TODO items), testing on try, and filing mozbase bugs where the API there needs improvement.

Ted, I'll endeavor to put a more complete patch up as soon as possible, but in the interim any feedback is appreciated.

Comment 47

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #44)
> > Sometime between attachment 797473 [details] [diff] [review] and
> > now I've introduced a bug where the websocket tests break :( The harness dies at
> > /tests/content/base/test/websocket_hybi/test_send-blob.html 
> 
> So part of the problem is the overloading of the websocket port in
> mozprofile.permissions.
> websocket port is messed up in mozprofile; it is (wrongly)
> set to the SSL proxy setting; see:
> -
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/
> permissions.py#L27
> vs
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> in?rev=a3e34b989ebe#52
> -
> https://github.com/mozilla/mozbase/commit/
> 43f9510e3d58bfed32790c82a57edac5f928474d
> 
>         diff pre/profile/ssltunnel.cfg now/profile/ssltunnel.cfg
>         2c2
>         < certdbdir:/home/jhammel/mozilla/src/mozilla-try/build/pgo/certs
>         ---
>         > certdbdir:/home/jhammel/mozilla/src/mozilla-central/build/pgo/certs
>         4c4
>         < websocketserver:127.0.0.1:9988
>         ---
>         > websocketserver:127.0.0.1:4443
> 
> Will file.
> 
> I've fixed this by using the old default in mochitest_options.py, but
> errors persist.  Will continue to troubleshoot.

This turns out not to be a problem with websockets at all, but instead with how I was using the timeout parameter.  Using this as outputTimeout in the call to mozrunner `start`, vs timeout (which would be maxTime in automation.py terms, but mochitest doesn't use this), yields the correct results. The timeout coincidentally corresponded to the websocket tests which had previously seen failures due to the issues in comment 44 on a particular machine, so I had assumed that the issues were related.

Comment 48

[Jeff Hammel]

In porting checkForZombies from automation.py to use in the `runApp` method,
I note that checkForZombies requires any number of helpers from automation.py:
 - isPidAlive
 - killAndGetStack
 - killAndGetStackNoScreenshot
 - dumpScreen
 - killPid

automation.py's `killAndGetStack` and `killAndGetStackNoScreenshot`
differ only by the fact that the former calls dumpscreen IFF
debuggerInfo is true:

http://hg.mozilla.org/mozilla-central/file/655ac375b1c7/build/automation.py.in#l676

http://hg.mozilla.org/mozilla-central/file/655ac375b1c7/build/automation.py.in#l683

I've combined the methods and introduced an additional boolean parameter,
`dump_screen`, that explicitly controls this behaviour for the one
place its called (`killAndGetStack` which is called from `checkForZombies`).

isPidAlive and killPid could be upstreamed to mozprocess.

dumpScreen could/should go to automationutils.py

This is a high cost of complexity to copy this code, but I see little
alternative.

Comment 49

[Jeff Hammel]

I noticed that my processOutputLine function was not getting hit; this is due to Bug 898379 not being uplifted to m-c.  Will work around for now, but we'll want this for m-c.

Comment 50

[Jeff Hammel]

Attachment: bug-746243

Still cleaning up the various bits of cleanup and output handling.  I put the line output handlers in a class so that I could keep state there; IMHO the best option, but am open to ideas.

The one bit that currently eludes me is the one bit reference to self.PERL in http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#726 . I'm not really sure what to do about this.  Again, ideas welcome.

Comment 51

[Jeff Hammel]

Attachment: interdiff.20130908232358.diff

changes from attachment 801163 [details] [diff] [review] to attachment 801391 [details] [diff] [review]

Comment 52

[(not currently active) Ted Mielczarek]

n.b.: I have a patch floating around that adds mozcrash.kill_and_get_minidump in bug 890026. Need to finish that off.

(In reply to Jeff Hammel [:jhammel] from comment #50)
> The one bit that currently eludes me is the one bit reference to self.PERL
> in http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#726
> . I'm not really sure what to do about this.  Again, ideas welcome.

PERL is only used for fix-linux-stack.pl:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#724

This is only used for local debug builds on Linux, so never in automation. I think as a stopgap we could just s/self.PERL/"perl"/. Longer-term it'd be great to port that code to Python and get rid of one of the hacks. It would also be nice to move all of the stack-fixing scripts to somewhere common, perhaps in mozbase, since the way we call them is extremely hacky right now.

Comment 53

[Jeff Hammel]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #52)
> n.b.: I have a patch floating around that adds
> mozcrash.kill_and_get_minidump in bug 890026. Need to finish that off.
> 
> (In reply to Jeff Hammel [:jhammel] from comment #50)
> > The one bit that currently eludes me is the one bit reference to self.PERL
> > in http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#726
> > . I'm not really sure what to do about this.  Again, ideas welcome.
> 
> PERL is only used for fix-linux-stack.pl:
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#724
> 
> This is only used for local debug builds on Linux, so never in automation. I
> think as a stopgap we could just s/self.PERL/"perl"/. Longer-term it'd be
> great to port that code to Python and get rid of one of the hacks. It would
> also be nice to move all of the stack-fixing scripts to somewhere common,
> perhaps in mozbase, since the way we call them is extremely hacky right now.

Filed bug 914253 ; I'll put up a something and we can iterate on it.  FWIW I think, e.g. mozstack is a good idea.

Comment 54

[Jeff Hammel]

Attachment: bug-746243

Nearly there; pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=66d01512c16d

Will do the last bits of cleanup and put up something for review.

Comment 55

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #54)
> Created attachment 801786 [details] [diff] [review]
> bug-746243
> 
> Nearly there; pushed to try:
> https://tbpl.mozilla.org/?tree=Try&rev=66d01512c16d
> 
> Will do the last bits of cleanup and put up something for review.

Erred out due to the which module being missed in production so I cancelled the run.  Will upload the fixed patch

Comment 56

[Jeff Hammel]

Attachment: bug-746243

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0f43f66b0bec

see http://k0s.org/mozilla/hg/mozilla-central-patches/rev/4189913f6ae6 for the only change since attachment 801786 [details] [diff] [review]

Comment 57

[(not currently active) Ted Mielczarek]

Comment on attachment 801864 [details] [diff] [review]
bug-746243

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

Overall this looks pretty sane. Will dive into it more thoroughly when it's r?, but I don't see anything I'm balking at. Then again, you're replacing automation.py, so the bar is set pretty low. :)

::: build/automationutils.py
@@ +10,5 @@
>  
> +try:
> +  import mozinfo
> +except ImportError:
> +  mozinfo = None

I think we should be able to unconditionally import this now.

@@ +355,5 @@
>    # otherwise just execute the command normally
>    return cmd
> +
> +class KeyValueParseError(Exception):
> +  """error when parsing strings of serialized key-values"""

As an aside, do you want to just write all this new code as 4-space indent, and we can fix the rest later?

@@ +377,5 @@
> +  # Note that whitespace is not stripped in exiting incarnation.
> +  return [string.split(separator, 1) for string in strings]
> +
> +def systemMemory():
> +  """returns total system memory in kilobytes"""

This might want documentation that this only works on *nix platforms.

@@ +382,5 @@
> +  return int(os.popen("free").readlines()[1].split()[1])
> +
> +def environment(xrePath, env=None, crashreporter=True):
> +  """populate OS environment variables for mochitest"""
> +  # From automation.py.in; this could be split into separate functions

Yeah, this is still kinda awful, but it's not your fault.

::: testing/mochitest/mach_commands.py
@@ +163,3 @@
>          options, args = opts.parse_args([])
>  
> +        appname = 'webapprt-stub' + mozinfo.info.get('bin_suffix', '')

Instead of stuffing bin_suffix into mozinfo, maybe we could just make a helper method somewhere? It's basically just "if windows, .exe, else empty".

::: testing/mochitest/mochitest_options.py
@@ +377,5 @@
>  
>          if options.symbolsPath and not isURL(options.symbolsPath):
>              options.symbolsPath = mochitest.getFullPath(options.symbolsPath)
>  
> +        # XXX override these, for some reason (cargo-culting) o_O

I think we can just move these into mochitest_options as defaults. They're just hardcoded anyway, might as well do it the right way.

::: testing/mochitest/runtests.py
@@ +826,5 @@
> +    tmpfd, processLog = tempfile.mkstemp(suffix='pidlog')
> +    os.close(tmpfd)
> +    env["MOZ_PROCESS_LOG"] = processLog
> +
> +    if is_test_build and self.runSSLTunnel:

I don't think you need self.runSSLTunnel, since we always want to run it for Mochitest.

@@ +891,5 @@
> +
> +    if onLaunch is not None:
> +      # Allow callers to specify an onLaunch callback to be fired after the
> +      # app is launched.
> +      onLaunch()

Is this going to be used somewhere?

Comment 58

[Jeff Hammel]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #57)
> Comment on attachment 801864 [details] [diff] [review]
> bug-746243
> 
> Review of attachment 801864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks pretty sane. Will dive into it more thoroughly when it's
> r?, but I don't see anything I'm balking at. Then again, you're replacing
> automation.py, so the bar is set pretty low. :)
> 
> ::: build/automationutils.py
> @@ +10,5 @@
> >  
> > +try:
> > +  import mozinfo
> > +except ImportError:
> > +  mozinfo = None
> 
> I think we should be able to unconditionally import this now.

Sadly, try disagrees for Android 4.0 Opt:

"""
ImportError: No module named mozinfo
Return code: 1
"""

Cool. Changed.
 
> @@ +355,5 @@
> >    # otherwise just execute the command normally
> >    return cmd
> > +
> > +class KeyValueParseError(Exception):
> > +  """error when parsing strings of serialized key-values"""
> 
> As an aside, do you want to just write all this new code as 4-space indent,
> and we can fix the rest later?

For ease of editing and my own sanity, I'd rather 

> @@ +377,5 @@
> > +  # Note that whitespace is not stripped in exiting incarnation.
> > +  return [string.split(separator, 1) for string in strings]
> > +
> > +def systemMemory():
> > +  """returns total system memory in kilobytes"""
> 
> This might want documentation that this only works on *nix platforms.

Added to docstring.

> ::: testing/mochitest/mach_commands.py
> @@ +163,3 @@
> >          options, args = opts.parse_args([])
> >  
> > +        appname = 'webapprt-stub' + mozinfo.info.get('bin_suffix', '')
> 
> Instead of stuffing bin_suffix into mozinfo, maybe we could just make a
> helper method somewhere? It's basically just "if windows, .exe, else empty".

Sounds good to me, but I'd rather take this as a separate bug if
that's okay.  I'd tend to put the helper still in mozinfo.

> ::: testing/mochitest/mochitest_options.py
> @@ +377,5 @@
> >
> >          if options.symbolsPath and not isURL(options.symbolsPath):
> >              options.symbolsPath = mochitest.getFullPath(options.symbolsPath)
> >
> > +        # XXX override these, for some reason (cargo-culting) o_O

> I think we can just move these into mochitest_options as defaults. They're
> just hardcoded anyway, might as well do it the right way.
> 
> ::: testing/mochitest/runtests.py
> @@ +826,5 @@
> > +    tmpfd, processLog = tempfile.mkstemp(suffix='pidlog')
> > +    os.close(tmpfd)
> > +    env["MOZ_PROCESS_LOG"] = processLog
> > +
> > +    if is_test_build and self.runSSLTunnel:
> 
> I don't think you need self.runSSLTunnel, since we always want to run it for
> Mochitest.

Fixed.

> @@ +891,5 @@
> > +
> > +    if onLaunch is not None:
> > +      # Allow callers to specify an onLaunch callback to be fired after the
> > +      # app is launched.
> > +      onLaunch()
>
> Is this going to be used somewhere?

No idea.  Pure cargo-culting.  I can delete if you want.

Comment 59

[Jonathan Griffin (:jgriffin)]

> > @@ +891,5 @@
> > > +
> > > +    if onLaunch is not None:
> > > +      # Allow callers to specify an onLaunch callback to be fired after the
> > > +      # app is launched.
> > > +      onLaunch()
> >
> > Is this going to be used somewhere?
> 
> No idea.  Pure cargo-culting.  I can delete if you want.

Please don't delete; it's used for running mochitests on b2g desktop builds.

Comment 60

[Joel Maher ( :jmaher ) (UTC -8)]

how about we add a clear comment about why this is needed, what bug is requiring this and what platforms depend on it.

Comment 61

[Jonathan Griffin (:jgriffin)]

(In reply to Joel Maher (:jmaher) from comment #60)
> how about we add a clear comment about why this is needed, what bug is
> requiring this and what platforms depend on it.

# Bug 826111 - We call onLaunch for b2g desktop mochitests so that we can
# run a Marionette script after gecko has completed startup.

Comment 62

[Joel Maher ( :jmaher ) (UTC -8)]

sold

Comment 63

[(not currently active) Ted Mielczarek]

I don't need a bug number in there, just the reasoning is useful, thanks!

Comment 64

[Jeff Hammel]

Attachment: bug-746243

This incorporates recommended changes and other various bug fixes.

While this appears to run successfully on e.g. Ubuntu, TBPL still reports this as a fail:  https://tbpl.mozilla.org/php/getParsedLog.php?id=27625640&tree=Try&full=1 . I suspect that this is due to log parsing e.g. encountering the string 'ERROR' or 'WARNING', which I suspect is due to mozprocess redirecting stderr -> stdout: https://bugzilla.mozilla.org/show_bug.cgi?id=794984 . 

The only other outstanding issue that I'm currently aware of and which causes me not to put this patch up for review is the piping done wrt stack fixing here:

http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#726

I'm trying to figure out how to do this in mozprocess-land, if possible.  Any pointers welcome.

Comment 65

[(not currently active) Ted Mielczarek]

All the variants of stack-fixer are fundamentally just output processors. They take a line of output, see if there's a symbol to resolve, and spit out a fixed line of output. That seems to fit pretty nicely with the mozprocess architecture, AIUI. That being said, having to use a separate process to do the perl nonsense might make that trickier.  I wonder if biting the bullet and porting fix-linux-stack.pl to Python would be less painful.

Comment 66

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #64)
> Created attachment 802495 [details] [diff] [review]
> bug-746243
> 
> This incorporates recommended changes and other various bug fixes.
> 
> While this appears to run successfully on e.g. Ubuntu, TBPL still reports
> this as a fail: 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27625640&tree=Try&full=1 .
> I suspect that this is due to log parsing e.g. encountering the string
> 'ERROR' or 'WARNING', which I suspect is due to mozprocess redirecting
> stderr -> stdout: https://bugzilla.mozilla.org/show_bug.cgi?id=794984 . 


For posterity the final part of the log:

01:33:40     INFO -  Mochitest INFO | 191770 INFO TEST-PASS | /tests/content/xml/document/test/test_bug343870.xhtml | undefined
01:33:40     INFO -  Mochitest INFO | 191771 INFO TEST-PASS | /tests/content/xml/document/test/test_bug343870.xhtml | undefined
01:33:40     INFO -  Mochitest INFO | 191772 INFO TEST-PASS | /tests/content/xml/document/test/test_bug343870.xhtml | undefined
01:33:40     INFO -  Mochitest INFO | 191773 INFO TEST-END | /tests/content/xml/document/test/test_bug343870.xhtml | finished in 83ms
01:33:40     INFO -  Mochitest INFO | 191774 INFO TEST-START | /tests/content/xml/document/test/test_bug355213.xhtml
01:33:40     INFO -  Mochitest INFO | 191775 INFO TEST-PASS | /tests/content/xml/document/test/test_bug355213.xhtml | Width should not change
01:33:40     INFO -  Mochitest INFO | 191776 INFO TEST-END | /tests/content/xml/document/test/test_bug355213.xhtml | finished in 69ms
01:33:40     INFO -  Mochitest INFO | 191777 INFO TEST-START | /tests/content/xml/document/test/test_bug392338.html
01:33:40     INFO -  Mochitest INFO | 191778 INFO TEST-PASS | /tests/content/xml/document/test/test_bug392338.html | No exception thrown
01:33:40     INFO -  Mochitest INFO | 191779 INFO TEST-PASS | /tests/content/xml/document/test/test_bug392338.html | Must be an interface requestor
01:33:40     INFO -  Mochitest INFO | 191780 INFO TEST-END | /tests/content/xml/document/test/test_bug392338.html | finished in 72ms
01:33:40     INFO -  Mochitest INFO | 191781 INFO TEST-START | /tests/content/xml/document/test/test_bug399502.xhtml
01:33:40     INFO -  Mochitest INFO | 191782 INFO TEST-PASS | /tests/content/xml/document/test/test_bug399502.xhtml | Onload handler didn't get called!
01:33:40     INFO -  Mochitest INFO | 191783 INFO TEST-END | /tests/content/xml/document/test/test_bug399502.xhtml | finished in 71ms
01:33:40     INFO -  Mochitest INFO | 191784 INFO TEST-START | /tests/content/xml/document/test/test_bug445330.html
01:33:40     INFO -  Mochitest INFO | 191785 INFO TEST-PASS | /tests/content/xml/document/test/test_bug445330.html | load event shouldn't have fired.
01:33:40     INFO -  Mochitest INFO | 191786 INFO TEST-PASS | /tests/content/xml/document/test/test_bug445330.html | DOMContentLoaded should have fired.
01:33:41     INFO -  Mochitest INFO | 191787 INFO TEST-END | /tests/content/xml/document/test/test_bug445330.html | finished in 71ms
01:33:41     INFO -  Mochitest INFO | 191788 INFO TEST-START | /tests/content/xml/document/test/test_bug691215.html
01:33:41     INFO -  Mochitest INFO | 191789 INFO TEST-PASS | /tests/content/xml/document/test/test_bug691215.html | Hey, we got here, that's good
01:33:41     INFO -  Mochitest INFO | 191790 INFO TEST-END | /tests/content/xml/document/test/test_bug691215.html | finished in 246ms
01:33:41     INFO -  Mochitest INFO | 191791 INFO TEST-START | /tests/content/xml/document/test/test_viewport.xhtml
01:33:41     INFO -  Mochitest INFO | 191792 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper width
01:33:41     INFO -  Mochitest INFO | 191793 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper height
01:33:41     INFO -  Mochitest INFO | 191794 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper initial scale
01:33:41     INFO -  Mochitest INFO | 191795 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper max scale
01:33:41     INFO -  Mochitest INFO | 191796 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper min scale
01:33:41     INFO -  Mochitest INFO | 191797 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper user scalable parameter
01:33:41     INFO -  Mochitest INFO | 191798 INFO TEST-END | /tests/content/xml/document/test/test_viewport.xhtml | finished in 82ms
01:33:41     INFO -  Mochitest INFO | 191799 INFO TEST-START | Shutdown
01:33:41     INFO -  Mochitest INFO | 191800 INFO Passed:  183760
01:33:41     INFO -  Mochitest INFO | 191801 INFO Failed:  0
01:33:41     INFO -  Mochitest INFO | 191802 INFO Todo:    940
01:33:41     INFO -  Mochitest INFO | 191803 INFO Slowest: 123209ms - /tests/content/events/test/test_wheel_default_action.html
01:33:41     INFO -  Mochitest INFO | 191804 INFO SimpleTest FINISHED
01:33:41     INFO -  Mochitest INFO | 191805 INFO TEST-INFO | Ran 1 Loops
01:33:41     INFO -  Mochitest INFO | 191806 INFO SimpleTest FINISHED
01:33:41     INFO -  Mochitest INFO | NOTE: child process received `Goodbye', closing down
01:33:42     INFO -  Mochitest INFO | [Parent 2383] WARNING: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed: 'glib warning', file ../../../toolkit/xre/nsSigHandlers.cpp, line 127
01:33:42     INFO -  Mochitest INFO |
01:33:42     INFO -  Mochitest INFO | (firefox:2383): Gtk-CRITICAL **: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed
01:33:42     INFO -  Mochitest INFO | [Parent 2383] WARNING: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed: 'glib warning', file ../../../toolkit/xre/nsSigHandlers.cpp, line 127
01:33:42     INFO -  Mochitest INFO |
01:33:42     INFO -  Mochitest INFO | (firefox:2383): Gtk-CRITICAL **: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed
01:33:42     INFO -  Mochitest INFO | INFO | runtests.py | exit 0
01:33:42     INFO -  Mochitest INFO | INFO | runtests.py | Application ran for: 0:33:35.869746
01:33:42     INFO -  Mochitest INFO | INFO | zombiecheck | Reading PID log: /tmp/tmp40zD_Kpidlog
01:33:42     INFO -  Mochitest INFO | ==> process 2383 launched child process 2455
01:33:42     INFO -  Mochitest INFO | ==> process 2383 launched child process 2469
01:33:42     INFO -  Mochitest INFO | ==> process 2383 launched child process 4230
01:33:42     INFO -  Mochitest INFO | INFO | zombiecheck | Checking for orphan process with PID: 2455
01:33:42     INFO -  Mochitest INFO | INFO | zombiecheck | Checking for orphan process with PID: 2469
01:33:42     INFO -  Mochitest INFO | INFO | zombiecheck | Checking for orphan process with PID: 4230
01:33:42     INFO -  Mochitest INFO | Zombie processes: False
01:33:42     INFO -  Mochitest INFO | Crashed: False
01:33:42     INFO -  Mochitest INFO | runtests.py | Running tests: end.
01:33:42     INFO - Return code: 0
01:33:42     INFO - TinderboxPrint: mochitest-plain1<br/><em class="testfail">T-FAIL</em>
01:33:42  WARNING - # TBPL WARNING #
01:33:42  WARNING - The mochitest suite: plain1 ran with return status: WARNING
01:33:42     INFO - Running post-action listener: _resource_record_post_action
01:33:42     INFO - Running post-run listener: _resource_record_post_run
01:33:43     INFO - Total resource usage - Wall time: 2050s; CPU: 70.0%; Read bytes: 59486208; Write bytes: 415604736; Read time: 11432; Write time: 883932
01:33:43     INFO - pull - Wall time: 16s; CPU: 94.0%; Read bytes: 0; Write bytes: 42237952; Read time: 0; Write time: 186112
01:33:43     INFO - install - Wall time: 17s; CPU: 100.0%; Read bytes: 0; Write bytes: 90468352; Read time: 0; Write time: 447816
01:33:43     INFO - run-tests - Wall time: 2018s; CPU: 69.0%; Read bytes: 59359232; Write bytes: 282824704; Read time: 11400; Write time: 250000
01:33:43     INFO - Running post-run listener: _upload_blobber_files
01:33:43  WARNING - Blob upload gear skipped. Missing cmdline options.
01:33:43     INFO - Copying logs to upload dir...
01:33:43     INFO - mkdir: /builds/slave/test/build/upload/logs
program finished with exit code 1
elapsedTime=2149.821181
========= Finished '/tools/buildbot/bin/python scripts/scripts/desktop_unittest.py ...' warnings (results: 1, elapsed: 35 mins, 52 secs) (at 2013-09-10 01:33:46.927616) =========

Comment 67

[Jeff Hammel]

> > @@ +355,5 @@
> > >    # otherwise just execute the command normally
> > >    return cmd
> > > +
> > > +class KeyValueParseError(Exception):
> > > +  """error when parsing strings of serialized key-values"""
> >
> > As an aside, do you want to just write all this new code as
> 4-space indent,
> > and we can fix the rest later?
> For ease of editing and my own sanity, I'd rather

Sorry, this got cut off.  Should be "...i'd rather do this as a separate bug."

Comment 68

[Jeff Hammel]

Attachment: mozprocess_test.py

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #65)
> All the variants of stack-fixer are fundamentally just output processors.
> They take a line of output, see if there's a symbol to resolve, and spit out
> a fixed line of output. That seems to fit pretty nicely with the mozprocess
> architecture, AIUI. That being said, having to use a separate process to do
> the perl nonsense might make that trickier.  I wonder if biting the bullet
> and porting fix-linux-stack.pl to Python would be less painful.

One way of looking at the problem as I see it is that mozprocess is architected in such a way that it is not, shall I say "convenient" to chain stdout to another process' stdin in the manner of subprocess: http://docs.python.org/2/library/subprocess.html .  At least, I don't see it. Also, ABICT the way to fill stdin of a subprocess is only via communicate() which takes a string.

Here's one of my naive attempts.  I would expect that this would work at least in some form output the lines.  The actual output follows. I'll play around with passing the stdout of (browser) process to the stack-fixer process.  I'll also consider rewriting the perl file.

Command:
ping -w 3 www.google.com
line: "PING www.google.com (74.125.239.112) 56(84) bytes of data."
didTimeout: "True"
newline: ""
line: "64 bytes from nuq05s01-in-f16.1e100.net (74.125.239.112): icmp_req=1 ttl=55 time=31.9 ms"
didTimeout: "True"
newline: ""
line: "64 bytes from nuq05s01-in-f16.1e100.net (74.125.239.112): icmp_req=2 ttl=55 time=20.5 ms"
didTimeout: "True"
newline: ""
line: "64 bytes from nuq05s01-in-f16.1e100.net (74.125.239.112): icmp_req=3 ttl=55 time=47.1 ms"
didTimeout: "True"
newline: ""
line: ""
didTimeout: "True"
newline: ""
line: "--- www.google.com ping statistics ---"
didTimeout: "True"
newline: ""
line: "3 packets transmitted, 3 received, 0% packet loss, time 2003ms"
didTimeout: "True"
newline: ""
line: "rtt min/avg/max/mdev = 20.551/33.205/47.106/10.878 ms"
didTimeout: "True"
newline: ""

Comment 69

[Jeff Hammel]

Attachment: mozprocess_test2.py

So this pattern works but with the considerable disadvantage that output doesn't seem to be processed until after program completion.  In addition, the architecture is "kinda janky".  I'll go forward with this way unless there are strong opinions to the contrary.

Comment 70

[Jonathan Griffin (:jgriffin)]

> I suspect that this is due to log parsing e.g. encountering the string
> 'ERROR' or 'WARNING', which I suspect is due to mozprocess redirecting
> stderr -> stdout: https://bugzilla.mozilla.org/show_bug.cgi?id=794984 . 

Mozharness does this also, so I'm not sure that's the cause of the false warning:

http://hg.mozilla.org/build/mozharness/file/4ecc13e691a6/mozharness/base/script.py#l663

Comment 71

[Jonathan Griffin (:jgriffin)]

When I run this patch locally, via mozharness, it doesn't exhibit this problem.  Maybe worth running on try again?

Comment 72

[Jonathan Griffin (:jgriffin)]

pushed the latest to try:  https://tbpl.mozilla.org/?tree=Try&rev=6167a66deb93

Comment 73

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #71)
> When I run this patch locally, via mozharness, it doesn't exhibit this
> problem.  Maybe worth running on try again?

And by "this problem", I mean the erroneous T-FAIL status.

Comment 74

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #72)
> pushed the latest to try: 
> https://tbpl.mozilla.org/?tree=Try&rev=6167a66deb93

Still failing.  One approach to debug this would be to check this into ash, and add some instrumentation to ash-mozharness to see why we're getting the T-FAIL status.

Comment 75

[Joel Maher ( :jmaher ) (UTC -8)]

06:19:13     INFO -  Mochitest INFO | INFO | runtests.py | exit 0
06:19:13     INFO -  Mochitest INFO | INFO | runtests.py | Application ran for: 0:33:12.589876
06:19:13     INFO -  Mochitest INFO | INFO | zombiecheck | Reading PID log: /tmp/tmp0Hnv1Lpidlog
06:19:13     INFO -  Mochitest INFO | ==> process 2336 launched child process 2410
06:19:13     INFO -  Mochitest INFO | ==> process 2336 launched child process 2424
06:19:13     INFO -  Mochitest INFO | ==> process 2336 launched child process 4167
06:19:13     INFO -  Mochitest INFO | INFO | zombiecheck | Checking for orphan process with PID: 2410
06:19:13     INFO -  Mochitest INFO | INFO | zombiecheck | Checking for orphan process with PID: 2424
06:19:13     INFO -  Mochitest INFO | INFO | zombiecheck | Checking for orphan process with PID: 4167
06:19:13     INFO -  Mochitest INFO | Zombie processes: False
06:19:13     INFO -  Mochitest INFO | Crashed: False
06:19:13     INFO -  Mochitest INFO | runtests.py | Running tests: end.
06:19:13     INFO - Return code: 0
06:19:13     INFO - TinderboxPrint: mochitest-plain1<br/><em class="testfail">T-FAIL</em>
06:19:13  WARNING - # TBPL WARNING #
06:19:13  WARNING - The mochitest suite: plain1 ran with return status: WARNING
06:19:13     INFO - Running post-action listener: _resource_record_post_action

we seem to be hitting this state:
http://hg.mozilla.org/build/mozharness/file/4ecc13e691a6/mozharness/mozilla/testing/unittest.py#l43

Looking into this it appears passed is never updated, on m-c, we have this:
06:52:11     INFO -  191774 INFO TEST-START | Shutdown
06:52:11     INFO -  191775 INFO Passed:  183659
06:52:11     INFO -  191776 INFO Failed:  0
06:52:11     INFO -  191777 INFO Todo:    942
06:52:11     INFO -  191778 INFO Slowest: 120133ms - /tests/content/events/test/test_wheel_default_action.html
06:52:11     INFO -  191779 INFO SimpleTest FINISHED
06:52:11     INFO -  191780 INFO TEST-INFO | Ran 1 Loops
06:52:11     INFO -  191781 INFO SimpleTest FINISHED

Here with this patch we have this:
06:19:12     INFO -  Mochitest INFO | 191688 INFO TEST-START | Shutdown
06:19:12     INFO -  Mochitest INFO | 191689 INFO Passed:  183698
06:19:12     INFO -  Mochitest INFO | 191690 INFO Failed:  0
06:19:12     INFO -  Mochitest INFO | 191691 INFO Todo:    938
06:19:12     INFO -  Mochitest INFO | 191692 INFO Slowest: 122083ms - /tests/content/events/test/test_wheel_default_action.html
06:19:12     INFO -  Mochitest INFO | 191693 INFO SimpleTest FINISHED
06:19:12     INFO -  Mochitest INFO | 191694 INFO TEST-INFO | Ran 1 Loops
06:19:12     INFO -  Mochitest INFO | 191695 INFO SimpleTest FINISHED


This is the regex used for summarizing:
        'regex': re.compile(r'''(\d+ INFO (Passed|Failed|Todo):\ +(\d+)|\t(Passed|Failed|Todo): (\d+))'''),


Is it possible that we have the extra "Mochitest INFO | " in the mozprocess way?  Could our logging module be inserting extra cruft?

Comment 76

[(not currently active) Ted Mielczarek]

Sounds pretty plausible. Sure wish we had structured logging all the way down. :-(

Comment 77

[Jonathan Griffin (:jgriffin)]

I can confirm this; I reproduced it locally, and self.pass_count and self.fail_count are still set to -1 here:  http://hg.mozilla.org/build/mozharness/file/e6c98cdad9d9/mozharness/mozilla/testing/unittest.py#l148

Comment 78

[Joel Maher ( :jmaher ) (UTC -8)]

I can't see in the patch that you pushed where you adjusted the log.  Maybe mozprocess does that internally for the stdout/stderr with the mozlog logger we have defined?  Could we mute that and push to try for a single test (i.e. not roll out 100 jobs to burn again)?

Alternatively, we could adjust the regex's for mozharness, but that sounds like the not so ideal approach.

Comment 79

[Jeff Hammel]

Thanks for all the detective work on my behalf here.  I certainly didn't mean to change logging but it appears to be a fallout of using the global log object already in runtests.py: http://hg.mozilla.org/mozilla-central/file/5417e5da2ceb/testing/mochitest/runtests.py#l32 . Changing 'Mochitest' to '' gives me:

d-blob.html | should have closed cleanly
 4:35.40 61726 INFO TEST-END | /tests/content/base/test/websocket_hybi/test_send-blob.html | finished in 148ms
 4:35.42 61727 INFO TEST-START | Shutdown
 4:35.42 61728 INFO Passed:  57224
 4:35.42 61729 INFO Failed:  0
 4:35.42 61730 INFO Todo:    151
 4:35.42 61731 INFO Slowest: 20964ms - /tests/content/base/test/test_mixed_content_blocker.html
 4:35.42 61732 INFO SimpleTest FINISHED

I'll try this on one platform and see what happens.

Comment 80

[Jeff Hammel]

pushed to linux opt: https://tbpl.mozilla.org/?tree=Try&rev=915c0ca240fe

Comment 81

[Jeff Hammel]

Attachment: bug-746243

This works locally, ABICT.  Will test on try if https://tbpl.mozilla.org/?tree=Try&rev=915c0ca240fe gives good results.

+        # TODO! websocket port is messed up in mozprofile; it is (wrongly)
+        # set to the SSL proxy setting; see:
+        #   - https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/permissions.py#L27
+        #     vs
+        #     http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in?rev=a3e34b989ebe#52
+        #   - https://github.com/mozilla/mozbase/commit/43f9510e3d58bfed32790c82a57edac5f928474d

^ Will ticket this for future work.

Comment 82

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #80)
> pushed to linux opt: https://tbpl.mozilla.org/?tree=Try&rev=915c0ca240fe

No luck; TBPL logs say:

11:47:56     INFO -  root INFO | 191746 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper min scale
11:47:56     INFO -  root INFO | 191747 INFO TEST-PASS | /tests/content/xml/document/test/test_viewport.xhtml | Should get proper user scalable parameter
11:47:56     INFO -  root INFO | 191748 INFO TEST-END | /tests/content/xml/document/test/test_viewport.xhtml | finished in 83ms
11:47:56     INFO -  root INFO | 191749 INFO TEST-START | Shutdown
11:47:56     INFO -  root INFO | 191750 INFO Passed:  183736
11:47:56     INFO -  root INFO | 191751 INFO Failed:  0
11:47:56     INFO -  root INFO | 191752 INFO Todo:    940
11:47:56     INFO -  root INFO | 191753 INFO Slowest: 122986ms - /tests/content/events/test/test_wheel_default_action.html
11:47:56     INFO -  root INFO | 191754 INFO SimpleTest FINISHED
11:47:56     INFO -  root INFO | 191755 INFO TEST-INFO | Ran 1 Loops
11:47:56     INFO -  root INFO | 191756 INFO SimpleTest FINISHED
11:47:57     INFO -  root INFO | NOTE: child process received `Goodbye', closing down
11:47:57     INFO -  root INFO | [Parent 2325] WARNING: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed: 'glib warning', file ../../../toolkit/xre/nsSigHandlers.cpp, line 127
11:47:57     INFO -  root INFO |
11:47:57     INFO -  root INFO | (firefox:2325): Gtk-CRITICAL **: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed
11:47:57     INFO -  root INFO | [Parent 2325] WARNING: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed: 'glib warning', file ../../../toolkit/xre/nsSigHandlers.cpp, line 127
11:47:57     INFO -  root INFO |
11:47:57     INFO -  root INFO | (firefox:2325): Gtk-CRITICAL **: IA__gtk_clipboard_get_for_display: assertion `!display->closed' failed
11:47:57     INFO -  root INFO | INFO | runtests.py | exit 0
11:47:57     INFO -  root INFO | INFO | runtests.py | Application ran for: 0:33:50.709294
11:47:57     INFO -  root INFO | INFO | zombiecheck | Reading PID log: /tmp/tmppS2hu6pidlog
11:47:57     INFO -  root INFO | ==> process 2325 launched child process 2394
11:47:57     INFO -  root INFO | ==> process 2325 launched child process 2409
11:47:57     INFO -  root INFO | ==> process 2325 launched child process 4149
11:47:57     INFO -  root INFO | INFO | zombiecheck | Checking for orphan process with PID: 2394
11:47:57     INFO -  root INFO | INFO | zombiecheck | Checking for orphan process with PID: 2409
11:47:57     INFO -  root INFO | INFO | zombiecheck | Checking for orphan process with PID: 4149
11:47:57     INFO -  root INFO | Zombie processes: False
11:47:57     INFO -  root INFO | Crashed: False
11:47:57     INFO -  root INFO | WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
11:47:57     INFO -  root INFO | runtests.py | Running tests: end.
11:47:57     INFO - Return code: 0
11:47:57     INFO - TinderboxPrint: mochitest-plain1<br/><em class="testfail">T-FAIL</em>
11:47:57  WARNING - # TBPL WARNING #
11:47:57  WARNING - The mochitest suite: plain1 ran with return status: WARNING

Comment 83

[Jeff Hammel]

Pushing with log code a la automation.py.in works: 

http://k0s.org/mozilla/hg/mozilla-central-patches/rev/00b495cef1e8

https://tbpl.mozilla.org/?tree=Try&rev=4426d768116b

Do we want to do this or should I persist in getting mozlog working as part of this bug?

Comment 84

[Jeff Hammel]

pushed a full try run with the above logger here: https://tbpl.mozilla.org/?tree=Try&rev=865026445dd8

Comment 85

[Jeff Hammel]

Attachment: bug-746243

A few minor fixes noted from the try run (minor in scope, big in effect).

I don't know what is up with the OS X failures.   Looking at them now.

Comment 86

[Jonathan Griffin (:jgriffin)]

This may be focus related; e.g., 

00:48:33     INFO -  890 INFO TEST-START | chrome://mochitests/content/chrome/content/base/test/chrome/test_bug571390.xul
00:48:37     INFO -  891 INFO Error: Unable to restore focus, expect failures and timeouts.

Comment 87

[Jeff Hammel]

Mozrunner is not adding -foreground to the arguments.  It should.  Trying with -foreground at https://tbpl.mozilla.org/?tree=Try&rev=2c0fadd722f7 though I'm not sure if this will suffice.

Comment 88

[Jeff Hammel]

With 33% of the precincts reporting, it looks like the missing -foreground flag is indeed the problem.  I'll see if this clears up 10.7 and 10.6 as well -- there is a comment that indicates that -foreground should be last, which it is not -- and ticket the issue for mozrunner as well as putting up a revised patch for this bug.

Comment 89

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #88)
> With 33% of the precincts reporting, it looks like the missing -foreground
> flag is indeed the problem.  I'll see if this clears up 10.7 and 10.6 as
> well -- there is a comment that indicates that -foreground should be last,
> which it is not -- and ticket the issue for mozrunner as well as putting up
> a revised patch for this bug.

Ticketed the mozrunner issue: bug 916512

Comment 90

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #81)
> Created attachment 803176 [details] [diff] [review]
> bug-746243
> 
> This works locally, ABICT.  Will test on try if
> https://tbpl.mozilla.org/?tree=Try&rev=915c0ca240fe gives good results.
> 
> +        # TODO! websocket port is messed up in mozprofile; it is (wrongly)
> +        # set to the SSL proxy setting; see:
> +        #   -
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/
> permissions.py#L27
> +        #     vs
> +        #    
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.
> in?rev=a3e34b989ebe#52
> +        #   -
> https://github.com/mozilla/mozbase/commit/
> 43f9510e3d58bfed32790c82a57edac5f928474d
> 
> ^ Will ticket this for future work.

Bug 916517

Comment 91

[Jeff Hammel]

From great-automation-refactor :
* Mirroring strategy, bug 746243 – port Mochitest to Mozbase https://bugzilla.mozilla.org/show_bug.cgi?id=746243 :- work around bugs in situ; so far, no hard blockers have been found- ticket upstream work (concurrently)- land m-c fix- fix pertinent upstream issues- bump versions & mirror -> m-c (in this case,  mozprofile, mozprocess, mozrunner)
https://etherpad.mozilla.org/great-automation-refactor

Comment 92

[Jeff Hammel]

Still failing on android.  Interesting lines:


"""
04:57:12     INFO -  Mochi-Remote INFO | Device info: {'uptime': ['0 days 0 hours 5 minutes 11 seconds 901 ms'], 'sutuserinfo': [], 'power': ['Power status:', ' AC power OFFLINE', ' Battery charge NO BATTERY', ' Remaining charge: 0%', ' Battery Temperature: 0.0 (c)'], 'process': [['10036', '1998', 'com.android.quicksearchbox'], ['10028', '1984', 'com.svox.pico'], ['10008', '1969', 'com.android.musicfx'], ['10027', '1939', 'com.android.defcontainer'], ['10045', '1509', 'net.rocboronat.android.wallpaper.npe'], ['10014', '1521', 'com.android.inputmethod.latin'], ['10007', '1614', 'com.android.smspush'], ['1001', '1547', 'com.android.phone'], ['1000', '1402', 'system'], ['10001', '1595', 'android.process.acore'], ['10017', '1954', 'com.android.gallery3d'], ['10038', '1710', 'com.android.providers.calendar'], ['10030', '1692', 'com.android.deskclock'], ['10021', '1837', 'com.android.calendar'], ['10044', '1882', 'com.mozilla.SUTAgentAndroid'], ['10024', '1567', 'com.android.launcher'], ['10000', '1494', 'android.process.media'], ['10031', '1726', 'com.android.email'], ['10033', '1742', 'com.android.exchange'], ['1000', '1814', 'com.android.settings'], ['10043', '1798', 'com.mozilla.watcher'], ['10032', '1757', 'com.android.mms'], ['1000', '1480', 'com.android.systemui'], ['10001', '1654', 'com.android.contacts']], 'screen': ['X:1280 Y:672'], 'memory': ['PA:764792832, FREE: 586006528'], 'systime': ['2013/09/16 04:57:10:519'], 'rotation': ['ROTATION:0'], 'disk': ['/data: 534761472 total, 393678848 available', '/system: 534761472 total, 185085952 available', '/mnt/sdcard: 8573165568 total, 8542478336 available'], 'os': ['pandaboard-eng 4.0.4 IMM76I 5 test-keys'], 'id': ['0e:60:2d:35:4e:01'], 'uptimemillis': ['311944'], 'temperature': ['Temperature: 38.2']}
04:57:12     INFO -  Mochi-Remote INFO | Test root: /mnt/sdcard/tests
04:57:12     INFO -  Mochi-Remote ERROR | Found a ',' in our value, unable to process value.
04:57:29     INFO -  Error deleting /data/anr/traces.txt
04:57:29     INFO -  runtests.py | Server pid: 28893
04:58:59     INFO -  /builds/panda-0805/test/build/hostutils/bin/xpcshell: error while loading shared libraries: libxpcom.so: cannot open shared object file: No such file or directory
04:58:59     INFO -  Timed out while waiting for server startup.
04:58:59     INFO -  Mochi-Remote ERROR | Automation Error: Exception caught while running tests
04:58:59     INFO -  Traceback (most recent call last):
04:58:59     INFO -    File "/builds/panda-0805/test/build/tests/mochitest/runtestsremote.py", line 693, in main
04:58:59     INFO -      retVal = mochitest.runTests(options)
04:58:59     INFO -    File "/builds/panda-0805/test/build/tests/mochitest/runtests.py", line 1002, in runTests
04:58:59     INFO -      self.startWebServer(options)
04:58:59     INFO -    File "/builds/panda-0805/test/build/tests/mochitest/runtestsremote.py", line 324, in startWebServer
04:58:59     INFO -      self.server.ensureReady(self.SERVER_STARTUP_TIMEOUT)
04:58:59     INFO -    File "/builds/panda-0805/test/build/tests/mochitest/runtests.py", line 184, in ensureReady
04:58:59     INFO -      sys.exit(1)
04:58:59     INFO -  SystemExit: 1
"""

I suspect either: A. we're not setting LD_LIBRARY_PATH (and we should be); or B. we think we are, but we're mysteriously erring out in buildRobotiumConfig for reason of a mysterious ','

Comment 93

[Joel Maher ( :jmaher ) (UTC -8)]

where is the commandline used for launch xpcshell?  you other assumptions are where I would start as well.

Comment 94

[(not currently active) Ted Mielczarek]

It's possible that we're just not setting the *right* LD_LIBRARY_PATH for Android runs, since they use an entirely separate host xpcshell which needs a different path for its libraries.

Comment 95

[Jeff Hammel]

Attachment: bug-746243 + debugging info

There's still the android issue, as evidenced by today's run: https://tbpl.mozilla.org/?tree=Try&rev=e4f2a0ec4736

Looking at logs now

Comment 96

[Jeff Hammel]

With :jmaher's considerable help, I was able to isolate the issue:  we're picking the wrong runner from mozrunner:

http://k0s.org/mozilla/hg/mozilla-central-patches/file/f884a314e1b0/bug-746243#l1170 . Since runremotetests uses runtests' code, we're getting runApp from (the refactored) runtests.py, not automation.py.in.  Since FennecRunner does not yet exist ( https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/remote.py#L334 ), instead runremotetests.py should use automation.py.in for the time being until FennecRunner becomes a priority

Comment 97

[Jeff Hammel]

From comment 58:
> > I think we can just move these into mochitest_options as
> defaults. They're
> > just hardcoded anyway, might as well do it the right way.
> >
> > ::: testing/mochitest/runtests.py
> > @@ +826,5 @@
> > > +    tmpfd, processLog = tempfile.mkstemp(suffix='pidlog')
> > > +    os.close(tmpfd)
> > > +    env["MOZ_PROCESS_LOG"] = processLog
> > > +
> > > +    if is_test_build and self.runSSLTunnel:
> >
> > I don't think you need self.runSSLTunnel, since we always want to
> run it for
> > Mochitest.
> Fixed.

Unfixed.  This is used by downstream consumers: http://mxr.mozilla.org/mozilla-central/search?string=runssltunnel

Comment 98

[Jeff Hammel]

Attachment: bug-746243

I added a front-end to runApp which I thought would be sufficient: http://k0s.org/mozilla/hg/mozilla-central-patches/rev/a23f215a494d .  Try, however, says no: https://tbpl.mozilla.org/?tree=Try&rev=e275cad7c447

There is still the browserEnv issue with commas:

"""
Mochi-Remote ERROR | buildRobotiumConfig: browserEnv - Found a ',' in our value, unable to process value. key=%s,value=%s
Mochi-Remote ERROR | browserEnv={'MOZ_CRASHREPORTER': '1', 'R_LOG_VERBOSE': '1', 'LOGNAME': 'cltbld', 'USER': 'cltbld', 'HOME': '/home/cltbld', 'PATH': '/usr/local/bin:/usr/local/bin:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/cltbld/bin', 'SUT_IP': '10.250.50.43', 'MINIDUMP_STACKWALK': '/builds/minidump_stackwalk', 'XPCOM_MEM_BLOAT_LOG': '/tmp/tmpsOyeDv/runtests_leaks.log', 'XPCOM_DEBUG_BREAK': 'stack', 'SHELL': '/bin/sh', 'SHLVL': '4', 'SUT_NAME': 'tegra-133', 'GNOME_DISABLE_CRASH_DIALOG': '1', 'NS_TRACE_MALLOC_DISABLE_STACKS': '1', 'PYTHONPATH': '/builds/sut_tools', 'R_LOG_LEVEL': '5', '_': '/tools/buildbot/bin/python2.7', 'MINIDUMP_SAVE_PATH': '/builds/tegra-133/test/minidumps', 'OLDPWD': '/home/cltbld', 'XRE_NO_WINDOWS_CRASH_DIALOG': '1', 'PWD': '/builds/tegra-133/test/build/tests', 'R_LOG_DESTINATION': 'stderr', 'MOZ_CRASHREPORTER_NO_REPORT': '1', 'NSPR_LOG_MODULES': 'signaling:5,mtransport:3'}
Error deleting /data/anr/traces.txt
runtests.py | Server pid: 10012
/builds/tegra-133/test/build/hostutils/bin/xpcshell: error while loading shared libraries: libxpcom.so: cannot open shared object file: No such file or directory
Timed out while waiting for server startup.
Mochi-Remote ERROR | Automation Error: Exception caught while running tests
Traceback (most recent call last):
  File "mochitest/runtestsremote.py", line 697, in main
  File "/builds/tegra-133/test/build/tests/mochitest/runtests.py", line 1002, in runTests
    self.startWebServer(options)
  File "mochitest/runtestsremote.py", line 324, in startWebServer
  File "/builds/tegra-133/test/build/tests/mochitest/runtests.py", line 183, in ensureReady
    sys.exit(1)
SystemExit: 1
"""

The comma here is in NSPR_LOG_MODULES:

>>> dict([(i, fleem[i]) for i in fleem if ',' in i or ',' in fleem[i]])
{'NSPR_LOG_MODULES': 'signaling:5,mtransport:3'}

Not sure if this is the only problem or if there are others.  Investigating.
"""

Comment 99

[Jeff Hammel]

> >>> dict([(i, fleem[i]) for i in fleem if ',' in i or ',' in
> fleem[i]]) {'NSPR_LOG_MODULES': 'signaling:5,mtransport:3'}

This is coming from automation.environment, which we are now occluding via putting environment in automationutils.py . I'll add end a front end to runtests.py/remoteruntests.py

Comment 100

[Jeff Hammel]

Thanks again to Joel, there is indeed another difference in the refactored Mochitest.runApp vs the automation.py.in version: namely, Mochitest.runApp passes the profile object around whereas automation.py's version expects just the path

Comment 101

[(not currently active) Ted Mielczarek]

Comment on attachment 803176 [details] [diff] [review]
bug-746243

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

Lots of comments, but nothing I would r- for. Thanks for taking on this Herculean task, I owe you many beers in return.

::: build/automationutils.py
@@ +11,5 @@
> +try:
> +  import mozinfo
> +except ImportError:
> +  # Stub out fake mozinfo since this is not importable on Android 4.0 Opt.
> +  # This should be fixed.

Can you find the bug on making Android tests run under mozharness and put that as the TODO comment here?

@@ +13,5 @@
> +except ImportError:
> +  # Stub out fake mozinfo since this is not importable on Android 4.0 Opt.
> +  # This should be fixed.
> +  mozinfo = type('mozinfo', (), dict(info={}))()
> +  mozinfo.isWin = mozinfo.isLinux = mozinfo.isUnix = mozinfo.isMac = False

This is sort of horrible, but since the only things using mozinfo are the things you're adding here it shouldn't be harmful. Can't wait till we actually have our virtualenv everywhere. :-/

@@ +376,5 @@
> +    raise KeyValueParseError("Error: syntax error in %s" % (context,
> +                                                            ','.join(missing)),
> +                                                            errors=missing)
> +
> +  # Note that whitespace is not stripped in exiting incarnation.

nit: "existing". Also this would probably be better off in the docstring.

@@ +384,5 @@
> +  """
> +  Returns total system memory in kilobytes.
> +  Works only on unix-like platforms where `free` is in the path.
> +  """
> +  return int(os.popen("free").readlines()[1].split()[1])

If we can actually rely on Python 2.7 everywhere (I'm fairly certain we can at this point), then you can use subprocess.check_output here:
http://docs.python.org/2/library/subprocess.html#subprocess.check_output

@@ +418,5 @@
> +  if crashreporter:
> +    env['MOZ_CRASHREPORTER_NO_REPORT'] = '1'
> +    env['MOZ_CRASHREPORTER'] = '1'
> +  else:
> +    env['MOZ_CRASHREPORTER_DISABLE'] = '1'

n.b.: I have a patch in bug 914925 (that got backed out) that touches thos.

@@ +427,5 @@
> +  # Also (temporary) bug 870002 (mediastreamgraph)
> +  env.setdefault('NSPR_LOG_MODULES', 'signaling:5,mtransport:3')
> +  env['R_LOG_LEVEL'] = '5'
> +  env['R_LOG_DESTINATION'] = 'stderr'
> +  env['R_LOG_VERBOSE'] = '1'

Hooray for forever-temporary!

::: testing/mochitest/mach_commands.py
@@ +154,5 @@
>          # all logging to go through us, we just remove their handler.
>          remove_handlers = [l for l in logging.getLogger().handlers
>              if isinstance(l, logging.StreamHandler)]
>          for handler in remove_handlers:
>              logging.getLogger().removeHandler(handler)

Is this necessary now that you're removing Automation from the mix?

@@ +169,2 @@
>          else:
> +            appname = os.path.join(self.distdir, 'bin', appname)

I know you're just shuffling existing code, but this block of code confuses me. Maybe you could move this into a separate function called get_webapp_runtime_path or something, and only call it in the blocks where it's needed below?

Also, this code ought to be able to just use MozbuildObject.get_binary_path, but you can punt that to a followup.

::: testing/mochitest/mochitest_options.py
@@ +377,5 @@
>  
>          if options.symbolsPath and not isURL(options.symbolsPath):
>              options.symbolsPath = mochitest.getFullPath(options.symbolsPath)
>  
> +        # XXX override these, for some reason (cargo-culting) o_O

If the defaults work then let's just use them. If not, file a followup on figuring out why and put the bug number in the comment here.

@@ +399,5 @@
> +        # 4c4
> +        # < websocketserver:127.0.0.1:9988
> +        # ---
> +        # > websocketserver:127.0.0.1:4443
> +        # """

This seems unnecessarily verbose. Just put TODO: websocket port is messed up in mozprofile; bug xxx.

::: testing/mochitest/runtests.py
@@ +6,5 @@
>  """
>  Runs the Mochitest test harness.
>  """
>  
>  from __future__ import with_statement

I guess we could drop this now that we require Python 2.7.

@@ +17,5 @@
>  import time
>  import traceback
>  
>  SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(__file__)))
>  sys.path.insert(0, SCRIPT_DIR);

I'd guess that now that you've removed the use of automation.py here the only thing keeping this around is automationutils.py. We should file a followup on making that get installed in the test package more like a mozbase module so that we don't have to do this. (It should already be in the in-tree virtualenv.)

@@ +33,5 @@
>  import mozinfo
> +import mozlog
> +import mozprocess
> +import mozrunner
> +import tempfile

I don't really understand why there are two separate lists of includes here, but could you at least put the system modules up with the others at the top?

@@ +96,5 @@
> +      # and re-raise any others
> +      if err.errno == errno.ESRCH or err.errno == errno.ECHILD:
> +        return False
> +      raise
> +# TODO: ^ upstream isPidAlive to mozprocess

n.b.: upon reading this code again I filed bug 917842 on it.

@@ +185,4 @@
>        if rtncode is None:
> +
> +        # TODO: need ProcessHandler.terminate() and/or .send_signal()
> +        # https://bugzilla.mozilla.org/show_bug.cgi?id=912285

Thanks for filing all these TODOs!

@@ +533,3 @@
>  
> +  # XXX use automation.py for test name to avoid breaking legacy
> +  # TODO: replace this with 'runtests.py' or 'mochitest' or the like

File a bug on this? I think the only thing we'd break is TBPL starring suggestions (which is kind of a big deal, but we can probably work around it).

@@ +616,2 @@
>  
> +    # BBB fix options.profilePath

what's the BBB?

@@ +702,5 @@
> +
> +    encoded = base64.b64encode(image)
> +    log.info("SCREENSHOT: data:image/png;base64,%s", encoded)
> +
> +  def killAndGetStack(self, processPID, utilityPath, debuggerInfo, dump_screen=False):

n.b.: bug 717758 (specifically attachment 805459 [details] [diff] [review]) changes this slightly and is likely to land today.

@@ +788,5 @@
>  
> +  def runApp(self, testUrl, env, app, profile, extraArgs,
> +             utilityPath, xrePath, certPath,
> +             debuggerInfo=None, symbolsPath=None,
> +             timeout=-1, onLaunch=None):

I kind of hate how our function declarations look when they've accumulated this many parameters. Is there a Pythonic way of formatting them? One-per-line?

@@ +812,5 @@
> +        ', '.join([("%s->%s" % (key, value)) for key, value in missing])))
> +      return 1
> +
> +    # fix default timeout
> +    if timeout == -1:

Future fodder, would be nice to have a better sigil value for "default timeout".

@@ +816,5 @@
> +    if timeout == -1:
> +      timeout = self.DEFAULT_TIMEOUT
> +
> +    # build parameters
> +    is_test_build = mozinfo.info.get('tests_enabled', True)

I think we should just ditch this is_test_build nonsense. You can't get to the point of running Mochitest without having tests enabled. (This probably made more sense in automation.py which got used in profileserver.py etc.)

@@ +829,5 @@
> +    env["MOZ_PROCESS_LOG"] = processLog
> +
> +    if is_test_build:
> +      # create certificate database for the profile
> +      # TODO: this should really be upstreamed somewhere, maybe mozprofile

If we upstream this I'd like to split up the "fill certificate DB" portion from the "setup ssltunnel config" portion.

Having the ssltunnel stuff somewhere shared might be nice.

@@ +872,5 @@
> +    # if the output handler is a pipe, it will process output via the subprocess
> +    if outputHandler.pipe:
> +      kp_kwargs = {}
> +    else:
> +      kp_kwargs = dict(processOutputLine=[outputHandler])

Any reason not to just write a dict literal?

@@ +874,5 @@
> +      kp_kwargs = {}
> +    else:
> +      kp_kwargs = dict(processOutputLine=[outputHandler])
> +
> +    # create mozrunner instance and start the SUT process

SUT process?

@@ +940,5 @@
> +    minidump_path = os.path.join(self.profile.profile, "minidumps")
> +    crashed = mozcrash.check_for_crashes(minidump_path,
> +                                         symbolsPath,
> +                                         test_name=self.lastTestSeen)
> +    log.info("Crashed: %s", crashed)

This seems superfluous, mozcrash will log something.

@@ +974,5 @@
>      if browserEnv is None:
>        return 1
>  
> +    # build profile sets self.profile
> +    # XXX this relies on sideeffects and isn't very stateful :(

bug number or GTFO

@@ +989,2 @@
>      self.startWebServer(options)
>      self.startWebSocketServer(options, debuggerInfo)

We should move the code that starts/stops ssltunnel into this function as well, to get all the server start/stop code into the same place. (I don't know why that's like it is now, probably just historical accident.) Followup is fine.

@@ +1180,5 @@
> +        index = line.find("=")
> +        if index != -1:
> +          self.browserProcessId = line[index+1:].rstrip()
> +          self.log.info("INFO | runtests.py | metro browser sub process id detected: %s", self.browserProcessId)
> +      return line

This whole output handling block is way nicer with mozprocess than it was before. Nice work!

@@ +1317,5 @@
> +        # for some reason, we ignore the first location
> +        # http://hg.mozilla.org/mozilla-central/file/21b5af569ca2/build/automation.py.in#l419
> +        # https://bugzilla.mozilla.org/show_bug.cgi?id=879740
> +        first = loc
> +        continue

Per that bug we ought to be able to just get rid of this.

Comment 102

[Jeff Hammel]

Attachment: /home/jhammel/mozilla/mozbase/bug-746243/mirroring/mirror.20130918012330.diff

Comment 103

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #102)
> Created attachment 807281 [details] [diff] [review]
> /home/jhammel/mozilla/mozbase/bug-746243/mirroring/mirror.20130918012330.diff

outstanding changes to m-c; tip of manifestdestiny mozdevice mozfile mozlog moznetwork mozprocess mozprofile mozrunner; try run: https://tbpl.mozilla.org/?tree=Try&rev=c20c67ab4e69

Comment 104

[Jeff Hammel]

comment 102 and comment 103 -> bug 917750

Comment 105

[Jeff Hammel]

will comment more in depth shortly, but not having mozinfo on android + LD_LIBRARY_PATH issues kill MochitestServer. Not necessarily final form, but pushed a hopeful fix at http://k0s.org/mozilla/hg/mozilla-central-patches/file/c26e5c1e2f5c/bug-746243 to try: https://tbpl.mozilla.org/?tree=Try&rev=6abbde96e36b

Comment 106

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #105)
> will comment more in depth shortly, but not having mozinfo on android +
> LD_LIBRARY_PATH issues kill MochitestServer. Not necessarily final form, but
> pushed a hopeful fix at
> http://k0s.org/mozilla/hg/mozilla-central-patches/file/c26e5c1e2f5c/bug-
> 746243 to try: https://tbpl.mozilla.org/?tree=Try&rev=6abbde96e36b

Traceback (most recent call last):
  File "mochitest/runtestsremote.py", line 17, in <module>
    from automation import Automation
  File "/builds/tegra-163/test/build/tests/mochitest/automation.py", line 22, in <module>
    import automationutils
  File "/builds/tegra-163/test/build/tests/mochitest/automationutils.py", line 27, in <module>
    attr = mapping.get(sys.platform)
AttributeError: 'list' object has no attribute 'get'
program finished with exit code 1

...and this is what you get when you copy+paste :/

Comment 107

[Jeff Hammel]

Attachment: bug-746243 : unbitrot, address review comments; semi-final form

Carrying r+ forward.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #101)
> Comment on attachment 803176 [details] [diff] [review]
> bug-746243
> 
> Review of attachment 803176 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lots of comments, but nothing I would r- for. Thanks for taking on this
> Herculean task, I owe you many beers in return.
> 
> ::: build/automationutils.py
> @@ +11,5 @@
> > +try:
> > +  import mozinfo
> > +except ImportError:
> > +  # Stub out fake mozinfo since this is not importable on Android 4.0 Opt.
> > +  # This should be fixed.
> 
> Can you find the bug on making Android tests run under mozharness and put
> that as the TODO comment here?

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=650881
 
> @@ +13,5 @@
> > +except ImportError:
> > +  # Stub out fake mozinfo since this is not importable on Android 4.0 Opt.
> > +  # This should be fixed.
> > +  mozinfo = type('mozinfo', (), dict(info={}))()
> > +  mozinfo.isWin = mozinfo.isLinux = mozinfo.isUnix = mozinfo.isMac = False
> 
> This is sort of horrible, but since the only things using mozinfo are the
> things you're adding here it shouldn't be harmful. Can't wait till we
> actually have our virtualenv everywhere. :-/

Likewise :/

> @@ +376,5 @@
> > +    raise KeyValueParseError("Error: syntax error in %s" % (context,
> > +                                                            ','.join(missing)),
> > +                                                            errors=missing)
> > +
> > +  # Note that whitespace is not stripped in exiting incarnation.
> 
> nit: "existing". Also this would probably be better off in the docstring.

Fixed.

> @@ +384,5 @@
> > +  """
> > +  Returns total system memory in kilobytes.
> > +  Works only on unix-like platforms where `free` is in the path.
> > +  """
> > +  return int(os.popen("free").readlines()[1].split()[1])
>
> If we can actually rely on Python 2.7 everywhere (I'm fairly certain we can
> at this point),

Heh, I'm not ;)

> then you can use subprocess.check_output here:
> http://docs.python.org/2/library/subprocess.html#subprocess.check_output

Indeed, or easy enough even < 2.7. This is a(n intentionally)
brain-dead copy-paste. We have psutil in tree which we should actually
use:
http://hg.mozilla.org/mozilla-central/file/59beb1868522/python/psutil
. If you want me to fix this herein I can or I can file a follow-up.

> @@ +418,5 @@
> > +  if crashreporter:
> > +    env['MOZ_CRASHREPORTER_NO_REPORT'] = '1'
> > +    env['MOZ_CRASHREPORTER'] = '1'
> > +  else:
> > +    env['MOZ_CRASHREPORTER_DISABLE'] = '1'
> 
> n.b.: I have a patch in bug 914925 (that got backed out) that touches thos.

Thanks for the heads up!

> @@ +427,5 @@
> > +  # Also (temporary) bug 870002 (mediastreamgraph)
> > +  env.setdefault('NSPR_LOG_MODULES', 'signaling:5,mtransport:3')
> > +  env['R_LOG_LEVEL'] = '5'
> > +  env['R_LOG_DESTINATION'] = 'stderr'
> > +  env['R_LOG_VERBOSE'] = '1'
> 
> Hooray for forever-temporary!

Srsly :(

> ::: testing/mochitest/mach_commands.py
> @@ +154,5 @@
> >          # all logging to go through us, we just remove their handler.
> >          remove_handlers = [l for l in logging.getLogger().handlers
> >              if isinstance(l, logging.StreamHandler)]
> >          for handler in remove_handlers:
> >              logging.getLogger().removeHandler(handler)
> 
> Is this necessary now that you're removing Automation from the mix?

See comment 83 and related;  I believe it is still necessary, though
gps would be the expert, I believe.

> 
> @@ +169,2 @@
> >          else:
> > +            appname = os.path.join(self.distdir, 'bin', appname)
> 
> I know you're just shuffling existing code, but this block of code confuses
> me. Maybe you could move this into a separate function called
> get_webapp_runtime_path or something, and only call it in the blocks where
> it's needed below?

Fixed.

> Also, this code ought to be able to just use MozbuildObject.get_binary_path,
> but you can punt that to a followup.
> 
> ::: testing/mochitest/mochitest_options.py
> @@ +377,5 @@
> >  
> >          if options.symbolsPath and not isURL(options.symbolsPath):
> >              options.symbolsPath = mochitest.getFullPath(options.symbolsPath)
> >  
> > +        # XXX override these, for some reason (cargo-culting) o_O
> 
> If the defaults work then let's just use them. If not, file a followup on
> figuring out why and put the bug number in the comment here.

I noticed after this draft that these are not part of the OptionParser
options, so in fact these are added here for convenience of variable
passing.  They are in fact part of the b2g options:

http://hg.mozilla.org/mozilla-central/file/8b4d14afc4f6/testing/mochitest/mochitest_options.py#l542

They should be made options.  I filed
bug 919315 for this.

> @@ +399,5 @@
> > +        # 4c4
> > +        # < websocketserver:127.0.0.1:9988
> > +        # ---
> > +        # > websocketserver:127.0.0.1:4443
> > +        # """
> 
> This seems unnecessarily verbose. Just put TODO: websocket port is messed up
> in mozprofile; bug xxx.

I hadn't ticketed yet when posting; now I have and note it:

http://k0s.org/mozilla/hg/mozilla-central-patches/file/af308229ba3e/bug-746243#l371

> ::: testing/mochitest/runtests.py
> @@ +17,5 @@
> >  import time
> >  import traceback
> >  
> >  SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(__file__)))
> >  sys.path.insert(0, SCRIPT_DIR);
> 
> I'd guess that now that you've removed the use of automation.py here the
> only thing keeping this around is automationutils.py. We should file a
> followup on making that get installed in the test package more like a
> mozbase module so that we don't have to do this. (It should already be in
> the in-tree virtualenv.)

There's a fair amount of insanity regarding SCRIPT_DIR and not only
the sys.path issue.  I'll file a follow up when I can.

> @@ +33,5 @@
> >  import mozinfo
> > +import mozlog
> > +import mozprocess
> > +import mozrunner
> > +import tempfile
> 
> I don't really understand why there are two separate lists of includes here,
> but could you at least put the system modules up with the others at the top?

I'll do one better and make there just one list.  It annoyed me too,
and was one of those things I had to sit-on-hands not to fix. (though
of course the from __future__ import and the sys and os imports will live
above the rest.)

> @@ +185,4 @@
> >        if rtncode is None:
> > +
> > +        # TODO: need ProcessHandler.terminate() and/or .send_signal()
> > +        # https://bugzilla.mozilla.org/show_bug.cgi?id=912285
> 
> Thanks for filing all these TODOs!

And only a million more to go ;)

> @@ +533,3 @@
> >  
> > +  # XXX use automation.py for test name to avoid breaking legacy
> > +  # TODO: replace this with 'runtests.py' or 'mochitest' or the like
> 
> File a bug on this? I think the only thing we'd break is TBPL starring
> suggestions (which is kind of a big deal, but we can probably work around
> it).

Will do.

> @@ +616,2 @@
> >  
> > +    # BBB fix options.profilePath
> 
> what's the BBB?

Like XXX but for backwards compatability; I had thought it was more
universal, but I suppose not since the top reference I could find was
plone: http://developer.plone.org/glossary.html .  I'll write a better
comment.

> @@ +702,5 @@
> > +
> > +    encoded = base64.b64encode(image)
> > +    log.info("SCREENSHOT: data:image/png;base64,%s", encoded)
 > > +
> > +  def killAndGetStack(self, processPID, utilityPath, debuggerInfo, dump_screen=False):
> 
> n.b.: bug 717758 (specifically attachment 805459 [details] [diff] [review])
> changes this slightly and is likely to land today.

Again, thanks for the heads up.  I adjusted the patch accordingly.

> @@ +788,5 @@
> >  
> > +  def runApp(self, testUrl, env, app, profile, extraArgs,
> > +             utilityPath, xrePath, certPath,
> > +             debuggerInfo=None, symbolsPath=None,
> > +             timeout=-1, onLaunch=None):
> 
> I kind of hate how our function declarations look when they've accumulated
> this many parameters. Is there a Pythonic way of formatting them?
> One-per-line?

Surprisingly, I couldn't find anything in PEP-8 to suggest a preferred
way (http://www.python.org/dev/peps/pep-0008/#indentation seems to
suggest it doesn't matter).  I'm of the school of "If the parameters
fit on the first line, then do that (probably).  Otherwise, one per
line."  I'll fix this to that pattern.

> @@ +812,5 @@
> > +        ', '.join([("%s->%s" % (key, value)) for key, value in missing])))
> > +      return 1
> > +
> > +    # fix default timeout
> > +    if timeout == -1:
> 
> Future fodder, would be nice to have a better sigil value for "default
> timeout".
> 
> @@ +816,5 @@
> > +    if timeout == -1:
> > +      timeout = self.DEFAULT_TIMEOUT
> > +
> > +    # build parameters
> > +    is_test_build = mozinfo.info.get('tests_enabled', True)
> 
> I think we should just ditch this is_test_build nonsense. You can't get to
> the point of running Mochitest without having tests enabled. (This probably
> made more sense in automation.py which got used in profileserver.py etc.)

Ditched, gladly.

> @@ +829,5 @@
> > +    env["MOZ_PROCESS_LOG"] = processLog
> > +
> > +    if is_test_build:
> > +      # create certificate database for the profile
> > +      # TODO: this should really be upstreamed somewhere, maybe mozprofile
> 
> If we upstream this I'd like to split up the "fill certificate DB" portion
> from the "setup ssltunnel config" portion.
> 
> Having the ssltunnel stuff somewhere shared might be nice.

Filed bug 919300 .
I'm happy to file a separate bug for ssltunnel if desired.

> @@ +872,5 @@
> > +    # if the output handler is a pipe, it will process output via the subprocess
> > +    if outputHandler.pipe:
> > +      kp_kwargs = {}
> > +    else:
> > +      kp_kwargs = dict(processOutputLine=[outputHandler])
> 
> Any reason not to just write a dict literal?

Nope.  Changed.  Is there a preference?

> @@ +874,5 @@
> > +      kp_kwargs = {}
> > +    else:
> > +      kp_kwargs = dict(processOutputLine=[outputHandler])
> > +
> > +    # create mozrunner instance and start the SUT process
> 
> SUT process?

System Under Test.  Now spelled out.

> @@ +940,5 @@
> > +    minidump_path = os.path.join(self.profile.profile, "minidumps")
> > +    crashed = mozcrash.check_for_crashes(minidump_path,
> > +                                         symbolsPath,
> > +                                         test_name=self.lastTestSeen)
> > +    log.info("Crashed: %s", crashed)
> 
> This seems superfluous, mozcrash will log something.

Sorry, debugging comment that I forgot to remove.  Removing it and a
few others.

> @@ +974,5 @@
> >      if browserEnv is None:
> >        return 1
> >  
> > +    # build profile sets self.profile
> > +    # XXX this relies on sideeffects and isn't very stateful :(
> 
> bug number or GTFO

Bug 919300 . Probably not worded well but, well, there you have it.

 
> @@ +989,2 @@
> >      self.startWebServer(options)
> >      self.startWebSocketServer(options, debuggerInfo)
> 
> We should move the code that starts/stops ssltunnel into this function as
> well, to get all the server start/stop code into the same place. (I don't
> know why that's like it is now, probably just historical accident.) Followup
> is fine.
> 
> @@ +1180,5 @@
> > +        index = line.find("=")
> > +        if index != -1:
> > +          self.browserProcessId = line[index+1:].rstrip()
> > +          self.log.info("INFO | runtests.py | metro browser sub process id detected: %s", self.browserProcessId)
> > +      return line
> 
> This whole output handling block is way nicer with mozprocess than it was
> before. Nice work!

Thanks!

> @@ +1317,5 @@
> > +        # for some reason, we ignore the first location
> > +        # http://hg.mozilla.org/mozilla-central/file/21b5af569ca2/build/automation.py.in#l419
> > +        # https://bugzilla.mozilla.org/show_bug.cgi?id=879740
> > +        first = loc
> > +        continue
> 
> Per that bug we ought to be able to just get rid of this.

Still haven't investigated this. Will endeavor to do so.

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=212c7e5ffe23

Looks pretty green.  Will do a last pass and do a full try push and then land unless anyone wants more out of this.

Comment 108

[Jeff Hammel]

Pushed a full try run: https://tbpl.mozilla.org/?tree=Try&rev=62e5d9c95c32 . One actual failure -- Moth on Win Opt -- (not seen previously because it only occurs on test timeout) was due to a variable name change.  It has been corrected: http://k0s.org/mozilla/hg/mozilla-central-patches/rev/29304a6a4f48#l1.28 .

Other than that....ABICT, things work fine.  I think I missed a few intended changes from http://hg.mozilla.org/mozilla-central/rev/de30dc2eacaa when unbitrotting, so I'll make sure I have those.  Then push -> m-i

Comment 109

[Jeff Hammel]

Attachment: bug-746243 : final form, i damn well hope

carrying r+ forward

Comment 110

[Jeff Hammel]

pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a733fffc6e9

Comment 111

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a733fffc6e9

Comment 112

[Marco Castelluccio [:marco]]

This broke the webapprt mach commands.

Comment 113

[Jeff Hammel]

(In reply to Marco Castelluccio [:marco] from comment #112)
> This broke the webapprt mach commands.

Could you elaborate or point to a bug?

Comment 114

[Marco Castelluccio [:marco]]

You can't run |mach webapprt-test-chrome| or |mach webapprt-test-content| anymore, because mozinfo isn't defined in the get_webapp_runtime_path function.

Comment 115

[Jeff Hammel]

Attachment: webapprt.diff

(In reply to Marco Castelluccio [:marco] from comment #114)
> You can't run |mach webapprt-test-chrome| or |mach webapprt-test-content|
> anymore, because mozinfo isn't defined in the get_webapp_runtime_path
> function.

This fixes.  Not sure if this is where you want the import, gps; originally this wasn't in a separate function, but on Ted's request I put it in one to make things more modular.  However, the import at http://hg.mozilla.org/mozilla-central/file/57d160eda301/testing/mochitest/mach_commands.py#l232 does not suffice now.

Comment 116

[Jeff Hammel]

(In reply to Marco Castelluccio [:marco] from comment #112)
> This broke the webapprt mach commands.

Traceback:

The details of the failure are as follows:

NameError: global name 'mozinfo' is not defined

  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 514, in run_mochitest_webapprt_chrome
    return self.run_mochitest(test_file, 'webapprt-chrome', **kwargs)
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 529, in run_mochitest
    **kwargs)
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 270, in run_desktop_test
    options.app = self.get_webapp_runtime_path()
  File "/home/jhammel/mozilla/src/mozilla-central/testing/mochitest/mach_commands.py", line 82, in get_webapp_runtime_path
    appname = 'webapprt-stub' + mozinfo.info.get('bin_suffix', '')

Comment 117

[Gregory Szorc [:gps]]

Comment on attachment 809358 [details] [diff] [review]
webapprt.diff

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

Sure.

FWIW, imports are one of the reasons I insist as much logic live outside mach_commands.py files as possible. If you have a standalone module, it can import everything in the global scope. You just import that module once from mach_commands.py inside the @Command method and you don't have to worry about global imports. Contrast with putting code in mach_commands.py, where each function may need its own imports. You can see how that gets unwieldy.

Comment 118

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #117)
> Comment on attachment 809358 [details] [diff] [review]
> webapprt.diff
> 
> Review of attachment 809358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sure.
> 
> FWIW, imports are one of the reasons I insist as much logic live outside
> mach_commands.py files as possible. If you have a standalone module, it can
> import everything in the global scope. You just import that module once from
> mach_commands.py inside the @Command method and you don't have to worry
> about global imports. Contrast with putting code in mach_commands.py, where
> each function may need its own imports. You can see how that gets unwieldy.

pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9781a20e996b

Comment 119

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9781a20e996b

Comment 120

[Ivan Alagenchev :ialagenchev]

I don't know if this one caused https://bugzilla.mozilla.org/show_bug.cgi?id=920728, but it seems related.

Comment 121

[Jeff Hammel]

(In reply to Ivan Alagenchev :ialagenchev from comment #120)
> I don't know if this one caused
> https://bugzilla.mozilla.org/show_bug.cgi?id=920728, but it seems related.

Indeed it does.  I'll look into it.

Comment 122

[Jeff Hammel]

Attachment: bug-902610.diff

disables the to_relative test and moves the problem to bug 920938

Comment 123

[Jeff Hammel]

Comment on attachment 811261 [details] [diff] [review]
bug-902610.diff

wrong bug