Enable some profile_chrome unit tests on Trybots

Enable some profile_chrome unit tests on Trybots,
specifically the ones that will currently run
without error.

This requires a sleep in between profile_chrome tests
to avoid a race condition that sometimes comes up
between unit tests. This also involves waking up
and unlocking the Android screen between every test.

BUG=catapult:#2748

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/aca5f2d4d1e963ffe1d57d804df67da5fa05ab55

[washingtonp, 2016-08-30 17:44:50]

Patchset #1 (id:1) has been deleted

[washingtonp, 2016-08-30 17:45:50]

Patchset #1 (id:20001) has been deleted

[washingtonp, 2016-08-30 18:16:45]

Patchset #1 (id:40001) has been deleted

[washingtonp, 2016-08-30 18:16:52]

Patchset #1 (id:60001) has been deleted

[washingtonp, 2016-09-01 22:49:55]

Description was changed from

==========
Sleep in between profile_chrome tests to avoid race condition

BUG=catapult:#2737
==========

to

==========
Sleep in between profile_chrome tests to avoid race condition

BUG=catapult:#2748
==========

[washingtonp, 2016-09-01 22:57:16]

Description was changed from

==========
Sleep in between profile_chrome tests to avoid race condition

BUG=catapult:#2748
==========

to

==========
Enable some profile_chrome unit tests on Trybots,
specifically the ones that will currently run
without error.

This requires a sleep in between profile_chrome tests
to avoid a race condition that sometimes comes up
between unit tests. This also involves waking up
and unlocking the Android screen between every test.

BUG=catapult:#2748
==========

[washingtonp, 2016-09-01 22:57:46]

washingtonp@google.com changed reviewers:
+ ccraik@google.com, skyostil@chromium.org, zhenw@chromium.org

[washingtonp, 2016-09-01 22:57:47]

ptal

[Zhen Wang, 2016-09-01 23:16:56]

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
File systrace/profile_chrome/agents_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
systrace/profile_chrome/agents_unittest.py:33: time.sleep(5)
Have you figured out how to know if the browser is up running, so we can avoid
this sleep?

https://codereview.chromium.org/2295913002/diff/160001/systrace/systrace/deco...
File systrace/systrace/decorators.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/systrace/deco...
systrace/systrace/decorators.py:20: def NeverRunTest(func):
Maybe rename to Disabled to be consistent?

[Sami, 2016-09-02 13:01:14]

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
File systrace/profile_chrome/agents_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
systrace/profile_chrome/agents_unittest.py:33: time.sleep(5)
On 2016/09/01 23:16:56, Zhen Wang wrote:
> Have you figured out how to know if the browser is up running, so we can avoid
> this sleep?

Yeah, it'd be great to avoid arbitrary sleeps since they might just make the
flake less likely but not completely eliminate it.

How does the test fail when we lose the race?

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
File systrace/profile_chrome/atrace_tracing_agent_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
systrace/profile_chrome/atrace_tracing_agent_unittest.py:18:
@decorators.NeverRunTest
For each completely disabled test, could you add a comment saying what needs to
be fixed so the test can run?

[Sami, 2016-09-02 13:01:45]

Thanks for doing this by the way! It's really difficult to catch regressions
without having these tests on the waterfall.

[washingtonp, 2016-09-08 22:13:02]

Patchset #7 (id:200001) has been deleted

[washingtonp, 2016-09-08 22:20:04]

Patchset #7 (id:220001) has been deleted

[washingtonp, 2016-09-08 22:51:37]

ptal

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
File systrace/profile_chrome/agents_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
systrace/profile_chrome/agents_unittest.py:33: time.sleep(5)
On 2016/09/01 23:16:56, Zhen Wang wrote:
> Have you figured out how to know if the browser is up running, so we can avoid
> this sleep?

Unfortunately, from what I have repeatedly observed on my
device, the browser has to not only be open, but the page
has to finish loading as well. I have not been able to find
a way to check this, but I can add that as a TODO.

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
systrace/profile_chrome/agents_unittest.py:33: time.sleep(5)
On 2016/09/02 13:01:13, Sami wrote:
> On 2016/09/01 23:16:56, Zhen Wang wrote:
> > Have you figured out how to know if the browser is up running, so we can
avoid
> > this sleep?
> 
> Yeah, it'd be great to avoid arbitrary sleeps since they might just make the
> flake less likely but not completely eliminate it.
> 
> How does the test fail when we lose the race?

The chrome agent tests fail to find the start trace marker
and so the timeout for the method is reached.

By the way, I have moved this logic to a util function since
it is now also called within the chrome agent (since the chrome
agent requires the user to have the browser already open).

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
File systrace/profile_chrome/atrace_tracing_agent_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/profile_chrom...
systrace/profile_chrome/atrace_tracing_agent_unittest.py:18:
@decorators.NeverRunTest
On 2016/09/02 13:01:13, Sami wrote:
> For each completely disabled test, could you add a comment saying what needs
to
> be fixed so the test can run?

Done.

https://codereview.chromium.org/2295913002/diff/160001/systrace/systrace/deco...
File systrace/systrace/decorators.py (right):

https://codereview.chromium.org/2295913002/diff/160001/systrace/systrace/deco...
systrace/systrace/decorators.py:20: def NeverRunTest(func):
On 2016/09/01 23:16:56, Zhen Wang wrote:
> Maybe rename to Disabled to be consistent?

Done.

[Sami, 2016-09-09 10:28:25]

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
File systrace/profile_chrome/chrome_tracing_agent.py (right):

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
systrace/profile_chrome/chrome_tracing_agent.py:43: curr_browser =
util.GetChromeProcessID(device, package_info)
I don't think the agents should be trying to start the browser. That is the
responsibility of the caller (i.e., test harness in this case).

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
File systrace/profile_chrome/perf_tracing_agent_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
systrace/profile_chrome/perf_tracing_agent_unittest.py:22: # TODO: Try enabling
this test for the SimpleperfProfilerAgent, which will
nit: User name missing.

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
File systrace/profile_chrome/util.py (right):

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
systrace/profile_chrome/util.py:40: def StartBrowser(device, package_info):
Feels like this fits in better in the test harness. Except for the startup test,
I don't think we'd need to start the browser automatically in any other case.

[washingtonp, 2016-09-09 18:07:29]

ptal

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
File systrace/profile_chrome/chrome_tracing_agent.py (right):

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
systrace/profile_chrome/chrome_tracing_agent.py:43: curr_browser =
util.GetChromeProcessID(device, package_info)
On 2016/09/09 10:28:25, Sami wrote:
> I don't think the agents should be trying to start the browser. That is the
> responsibility of the caller (i.e., test harness in this case).

Done.

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
File systrace/profile_chrome/perf_tracing_agent_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
systrace/profile_chrome/perf_tracing_agent_unittest.py:22: # TODO: Try enabling
this test for the SimpleperfProfilerAgent, which will
On 2016/09/09 10:28:25, Sami wrote:
> nit: User name missing.

Done.

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
File systrace/profile_chrome/util.py (right):

https://codereview.chromium.org/2295913002/diff/280001/systrace/profile_chrom...
systrace/profile_chrome/util.py:40: def StartBrowser(device, package_info):
On 2016/09/09 10:28:25, Sami wrote:
> Feels like this fits in better in the test harness. Except for the startup
test,
> I don't think we'd need to start the browser automatically in any other case.

Done.

[Sami, 2016-09-12 14:26:32]

lgtm, thanks!

https://codereview.chromium.org/2295913002/diff/320001/systrace/profile_chrom...
File systrace/profile_chrome/agents_unittest.py (right):

https://codereview.chromium.org/2295913002/diff/320001/systrace/profile_chrom...
systrace/profile_chrome/agents_unittest.py:22: if curr_browser == None:
nit: prefer "is None" instead of "== None" (or just "if not curr_browser")
(ditto in the other cases below)

[Zhen Wang, 2016-09-12 16:48:13]

lgtm

[Chris Craik, 2016-09-12 17:34:05]

systrace changes LGTM

[washingtonp, 2016-09-12 17:37:01]

The CQ bit was checked by washingtonp@google.com

[commit-bot: I haz the power, 2016-09-12 17:37:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 17:46:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 17:46:39]

Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)

[washingtonp, 2016-09-12 19:17:53]

The CQ bit was checked by washingtonp@google.com

[washingtonp, 2016-09-12 19:17:53]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org,
ccraik@google.com, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2295913002/#ps360001
(title: "Refactored profiler unittests to work with merged in refactored
profile_chrome controller")

[commit-bot: I haz the power, 2016-09-12 19:17:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 19:33:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 19:33:21]

Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[washingtonp, 2016-09-12 19:43:34]

On 2016/09/12 19:33:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

There is currently no OWNER of systrace/bin/run_tests is; is anyone willing to
be
added as an OWNER?

[Chris Craik, 2016-09-12 19:55:04]

On 2016/09/12 at 19:43:34, washingtonp wrote:
> On 2016/09/12 19:33:21, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
> >
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
> 
> There is currently no OWNER of systrace/bin/run_tests is; is anyone willing to
be
> added as an OWNER?

I am, though I'd welcome others too.

[washingtonp, 2016-09-12 20:02:14]

The CQ bit was checked by washingtonp@google.com

[washingtonp, 2016-09-12 20:02:15]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org,
ccraik@google.com, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2295913002/#ps380001
(title: "Add Chris as owner of Systrace bin directory")

[commit-bot: I haz the power, 2016-09-12 20:02:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 20:23:54]

Description was changed from

==========
Enable some profile_chrome unit tests on Trybots,
specifically the ones that will currently run
without error.

This requires a sleep in between profile_chrome tests
to avoid a race condition that sometimes comes up
between unit tests. This also involves waking up
and unlocking the Android screen between every test.

BUG=catapult:#2748
==========

to

==========
Enable some profile_chrome unit tests on Trybots,
specifically the ones that will currently run
without error.

This requires a sleep in between profile_chrome tests
to avoid a race condition that sometimes comes up
between unit tests. This also involves waking up
and unlocking the Android screen between every test.

BUG=catapult:#2748

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-09-12 20:23:55]

Committed patchset #14 (id:380001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[washingtonp, 2016-09-12 20:25:22]

On 2016/09/12 20:23:55, commit-bot: I haz the power wrote:
> Committed patchset #14 (id:380001) as
>
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

Thanks Chris. I added you as the OWNER for now. If anyone else
would like to be added/replaced, we can change the
OWNER file.