Add support for IsApplicationRunning on Mac.

Add support for IsApplicationRunning on Mac.

PosixPlatformBackend didn't have support for detecting if an app is
running.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278524

[baxley, 2014-06-18 00:45:49]

I moved some of the logic to check if an app is running to
posix_platform_backend.py

I tested this with the other CL (https://codereview.chromium.org/253903004/).
Let me know what you think.

https://codereview.chromium.org/331843007/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/331843007/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/__init__.py:136: return
self._platform_backend.IsApplicationRunning(application)
There isn't a method "IsApplicationLaunchning" in platform_backend.py, but there
is "IsApplicationRunning". Let me know if this is incorrect.

https://codereview.chromium.org/331843007/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/331843007/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:76: return
any(application in cmd for cmd in ps_output)
This just checks if the app is a substring of the command returned from ps, so
let me know if this is too permissive.

[baxley, 2014-06-18 18:27:21]

I made the check a little more strict so that the app name being searched for in
ps must match a complete string starting from the beginning (or after a slash).
After the string, there should be the end of the string (or a space).

[dtu, 2014-06-18 21:06:42]

Great! lgtm

[tonyg, 2014-06-18 21:13:40]

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:136: return
self._platform_backend.IsApplicationRunning(application)
Good fix. This points out a complete lack of test coverage for this method.
Would you be willing to add a unittest?

Maybe something as simple as:
  assertFalse(IsApplicationRunning('Nonexistent'))
  assertTrue(IsApplicationRunning(sys.executable))

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:75: ps_output
= self._GetPsOutput(['command'], pid=None)
You can just omit pid=None.

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:76:
application_re = re.compile('(.*/|^)%s(\s|$)' % application)
We might need to test that executables with spaces in the name work. For example
"Google Chrome" on the mac.

Also, should the '/' be os.pathsep?

[baxley, 2014-06-18 23:01:26]

I initially had the unit tests in posix_platform_backend_unittest.py, but I
wasn't sure how to filter the tests for only posix supporting platforms. Using
the mock backend in that unit test didn't work, since it mocks out _GetPsOutput,
which my code uses.

I moved it into mac_platform_backend_unittest where I use the OS platform
backend, and I could do test.Enabled('mac')

If you see any issues or have a better place to put the test, let me know.

Thanks for the feedback.
-Mike

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:136: return
self._platform_backend.IsApplicationRunning(application)
On 2014/06/18 21:13:40, tonyg wrote:
> Good fix. This points out a complete lack of test coverage for this method.
> Would you be willing to add a unittest?
> 
> Maybe something as simple as:
>   assertFalse(IsApplicationRunning('Nonexistent'))
>   assertTrue(IsApplicationRunning(sys.executable))

No problem. I had written one locally and thrown it away since I wasn't sure if
there was anything we could count on running. But sys.executable sounds good.

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:75: ps_output
= self._GetPsOutput(['command'], pid=None)
On 2014/06/18 21:13:40, tonyg wrote:
> You can just omit pid=None.

Done.

https://codereview.chromium.org/331843007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:76:
application_re = re.compile('(.*/|^)%s(\s|$)' % application)
On 2014/06/18 21:13:40, tonyg wrote:
> We might need to test that executables with spaces in the name work. For
example
> "Google Chrome" on the mac.
I tested this locally and it worked. I'm not sure of a reliable app to look for
that we could add this to the unit test.

> 
> Also, should the '/' be os.pathsep?
Done.

[tonyg, 2014-06-19 00:28:35]

On 2014/06/18 23:01:26, baxley wrote:
> I initially had the unit tests in posix_platform_backend_unittest.py, but I
> wasn't sure how to filter the tests for only posix supporting platforms. Using
> the mock backend in that unit test didn't work, since it mocks out
_GetPsOutput,
> which my code uses.
> 
> I moved it into mac_platform_backend_unittest where I use the OS platform
> backend, and I could do test.Enabled('mac')
> 
> If you see any issues or have a better place to put the test, let me know.

Let's move it back to posix_platform_backend_unittest.py and add
@test.Enabled('linux', 'mac', 'win')

With that, this change lgtm and the CQ should let us know if we did anything
wrong on any of the above 3 platforms.

[baxley, 2014-06-19 14:17:09]

The CQ bit was checked by baxley@chromium.org

[commit-bot: I haz the power, 2014-06-19 14:19:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/baxley@chromium.org/331843007/60001

[baxley, 2014-06-19 15:51:57]

On 2014/06/19 00:28:35, tonyg wrote:
> On 2014/06/18 23:01:26, baxley wrote:
> > I initially had the unit tests in posix_platform_backend_unittest.py, but I
> > wasn't sure how to filter the tests for only posix supporting platforms.
Using
> > the mock backend in that unit test didn't work, since it mocks out
> _GetPsOutput,
> > which my code uses.
> > 
> > I moved it into mac_platform_backend_unittest where I use the OS platform
> > backend, and I could do test.Enabled('mac')
> > 
> > If you see any issues or have a better place to put the test, let me know.
> 
> Let's move it back to posix_platform_backend_unittest.py and add
> @test.Enabled('linux', 'mac', 'win')
> 
> With that, this change lgtm and the CQ should let us know if we did anything
> wrong on any of the above 3 platforms.

The CQ failed on win_chromium, since IsApplicationRunning isn't implemented on
that platform. WinPlatformBackend isn't a subclass of PosixPlatformBackend. It
probably needs to be implemented in win_platform_backend, so for now I'm
removing 'win' from the test, so we'll at least have it for mac and linux. I'll
file a bug for adding support on Windows.

[baxley, 2014-06-19 15:52:20]

The CQ bit was checked by baxley@chromium.org

[commit-bot: I haz the power, 2014-06-19 15:53:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/baxley@chromium.org/331843007/80001

[commit-bot: I haz the power, 2014-06-19 23:37:56]

Change committed as 278524