telemetry: handle tests that are not supported on a platform.

telemetry: handle tests that are not supported on a platform.

When a test or measurement was not supported on a platform, we used to print
a warning and call sys.exit(0). This caused problems with unittests, because
it was considered a failure (for page measurements it was not a failure).

This patch introduces the page_test.TestNotSupportedOnPlatformFailure
exception. Tests and measurements throw this exception if they can't run on
a platform. PageTestRunner and unit tests catch these exceptions and
print a warning, but they don't consider it an error.

R=tonyg@chromium.org, dtu@chromium.org, vmpstr@chromium.org
BUG=

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

[dtu, 2013-12-20 19:28:01]

https://codereview.chromium.org/100443007/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_test_runner.py (right):

https://codereview.chromium.org/100443007/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_test_runner.py:45: except
page_test_module.TestNotSupportedOnPlatformFailure as failure:
Not here. There's a big exception handling block in page_runner.py that handles
these exceptions. But since TestNotSupportedOnPlatformFailure is a subclass of
Failure, I think it's already handled.

[vmpstr, 2013-12-20 23:20:10]

rasterize_and_record_micro* lgtm with nits

https://codereview.chromium.org/100443007/diff/1/tools/perf/measurements/rast...
File tools/perf/measurements/rasterize_and_record_micro.py (right):

https://codereview.chromium.org/100443007/diff/1/tools/perf/measurements/rast...
tools/perf/measurements/rasterize_and_record_micro.py:44: # Check if the we
actually have threaded forced compositing enabled.
nit: s/the // and possibly s/forced //

https://codereview.chromium.org/100443007/diff/1/tools/perf/measurements/rast...
tools/perf/measurements/rasterize_and_record_micro.py:47: and
system_info.gpu.feature_status.get(
nit: I don't really know the (chromium) python style.. should "and" be at the
end of the previous line? It seems to be more consistent with if around line 61

https://codereview.chromium.org/100443007/diff/1/tools/perf/measurements/rast...
tools/perf/measurements/rasterize_and_record_micro.py:48: 'compositing', None)
== 'enabled_force_threaded'):
I think None is not needed here, assuming feature_status is a dict

[ernstm, 2013-12-20 23:25:59]

https://codereview.chromium.org/100443007/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_test_runner.py (right):

https://codereview.chromium.org/100443007/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_test_runner.py:45: except
page_test_module.TestNotSupportedOnPlatformFailure as failure:
On 2013/12/20 19:28:01, dtu wrote:
> Not here. There's a big exception handling block in page_runner.py that
handles
> these exceptions. But since TestNotSupportedOnPlatformFailure is a subclass of
> Failure, I think it's already handled.

We can't handle the exception in PageRunner, because the unit tests would fail
(it would get an empty result and complain about missing metrics). The
try/except is wrapped around page_runner.Run, because this will get called from
both run_measurement and the unit tests and those want to handle the exception
differently. For run_measuremnt, we want to return an empty results and in the
unit test we just want to return and make the test pass. The exception handling
block in PageRunner only catches the TestNotSupportedOnPlaformFailure and raises
it again so that the caller can handle it.

[dtu, 2013-12-21 00:07:14]

On 2013/12/20 23:25:59, Manfred Ernst wrote:
>
https://codereview.chromium.org/100443007/diff/1/tools/telemetry/telemetry/pa...
> File tools/telemetry/telemetry/page/page_test_runner.py (right):
> 
>
https://codereview.chromium.org/100443007/diff/1/tools/telemetry/telemetry/pa...
> tools/telemetry/telemetry/page/page_test_runner.py:45: except
> page_test_module.TestNotSupportedOnPlatformFailure as failure:
> On 2013/12/20 19:28:01, dtu wrote:
> > Not here. There's a big exception handling block in page_runner.py that
> handles
> > these exceptions. But since TestNotSupportedOnPlatformFailure is a subclass
of
> > Failure, I think it's already handled.
> 
> We can't handle the exception in PageRunner, because the unit tests would fail
> (it would get an empty result and complain about missing metrics). The
> try/except is wrapped around page_runner.Run, because this will get called
from
> both run_measurement and the unit tests and those want to handle the exception
> differently. For run_measuremnt, we want to return an empty results and in the
> unit test we just want to return and make the test pass. The exception
handling
> block in PageRunner only catches the TestNotSupportedOnPlaformFailure and
raises
> it again so that the caller can handle it.


A few gotchas here:
- run_measurement and run_benchmark currently have different code paths -
run_benchmark uses a class called TestRunner, which calls PageRunner directly,
circumventing PageTestRunner. So changes to PageTestRunner won't be seen from
the bots using run_benchmark. I have a bug open to merge the two. (Sorry, I know
this is super confusing.)
- PageTestRunner/TestRunner is focused entirely on parsing the command-line and
figuring out what to run. The responsibility for running the test and handling
errors is in PageRunner.
- For run_measurement/run_benchmark, I don't think we want to return empty
results - the entire test run is invalid, so we should raise an error.

Overall, I think it's fine if we just don't catch the
TestNotSupportedOnPlaformFailure in PageTestResults, and let the exception go
straight to the user.

[ernstm, 2014-03-11 17:08:06]

> A few gotchas here:
> - run_measurement and run_benchmark currently have different code paths -
> run_benchmark uses a class called TestRunner, which calls PageRunner directly,
> circumventing PageTestRunner. So changes to PageTestRunner won't be seen from
> the bots using run_benchmark. I have a bug open to merge the two. (Sorry, I
know
> this is super confusing.)
> - PageTestRunner/TestRunner is focused entirely on parsing the command-line
and
> figuring out what to run. The responsibility for running the test and handling
> errors is in PageRunner.
> - For run_measurement/run_benchmark, I don't think we want to return empty
> results - the entire test run is invalid, so we should raise an error.
> 
> Overall, I think it's fine if we just don't catch the
> TestNotSupportedOnPlaformFailure in PageTestResults, and let the exception go
> straight to the user.

Picking up this CL again.

Rebased and addressed the issue with run_benchmark vs. run_measurement pointed
out by Dave.

The patch doesn't make TestNotSupportedOnPlatformFailure a hard failure.
Benchmarks and measurements simply skip the measurement and leave the result
empty when this exception is raised. That is the behavior we currently have
implemented deeply in the measurement itself, where MeasurePage silently exits.
The exception makes this a bit cleaner.

Unfortunately, we can't handle the exception in PageRunner, because that won't
work with the unit tests. The unit tests check if all metrics are present in the
results. If the exception was handled in PageRunner, they would get empty
results and fail. By re-raising the exception, we give the unit test the
opportunity to handle the exception itself and succeed. I'd say that this is the
correct behavior, as the measurement correctly tells us that it can't run on the
current platform/system/chrome_version and we should just ignore it. A warning
is still issued.

The reasoning behind all this is that we can't statically determine where a
measurement is supported. E.g. rasterize_and_recrod requires a minimum Chrome
version and impl-side painting enabled. We won't know until we start the
browser. Our Linux perf bots don't support imple-side painting, but disabling
the benchmark on all Linux machines (as we do right now) is overkill.

[ernstm, 2014-03-14 00:42:29]

Ping

[tonyg, 2014-03-14 01:32:49]

lgtm

https://codereview.chromium.org/100443007/diff/90001/tools/perf/measurements/...
File tools/perf/measurements/rasterize_and_record.py (right):

https://codereview.chromium.org/100443007/diff/90001/tools/perf/measurements/...
tools/perf/measurements/rasterize_and_record.py:64: # Throw and exception if
threaded forced compositing is not enabled.
s/and/an/

Or actually, just delete the comment altogether. It doesn't add much value.

https://codereview.chromium.org/100443007/diff/90001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/100443007/diff/90001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:56: class
TestNotSupportedOnPlatformFailure(Failure):
Dave know this better, and I'm not exactly sure, but does this belong in test.py
instead of page_test.py?

[ernstm, 2014-03-24 22:27:21]

Dave, does the TestNotSupportedOnPlatformFailure belong into test.py or
page_test.py?

https://codereview.chromium.org/100443007/diff/90001/tools/perf/measurements/...
File tools/perf/measurements/rasterize_and_record.py (right):

https://codereview.chromium.org/100443007/diff/90001/tools/perf/measurements/...
tools/perf/measurements/rasterize_and_record.py:64: # Throw and exception if
threaded forced compositing is not enabled.
On 2014/03/14 01:32:49, tonyg wrote:
> s/and/an/
> 
> Or actually, just delete the comment altogether. It doesn't add much value.

Done.

[dtu, 2014-03-27 23:21:59]

On 2014/03/24 22:27:21, ernstm wrote:
> Dave, does the TestNotSupportedOnPlatformFailure belong into test.py or
> page_test.py?

Either is okay. Leaning towards both Failure and
TestNotSupportedOnPlatformFailure should be in test.py.
> 
>
https://codereview.chromium.org/100443007/diff/90001/tools/perf/measurements/...
> File tools/perf/measurements/rasterize_and_record.py (right):
> 
>
https://codereview.chromium.org/100443007/diff/90001/tools/perf/measurements/...
> tools/perf/measurements/rasterize_and_record.py:64: # Throw and exception if
> threaded forced compositing is not enabled.
> On 2014/03/14 01:32:49, tonyg wrote:
> > s/and/an/
> > 
> > Or actually, just delete the comment altogether. It doesn't add much value.
> 
> Done.

[dtu, 2014-03-27 23:22:09]

lgtm with nits

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_runner.py:484: raise failure
Python shorthand:
 483   except page_test.TestNotSupportedOnPlatformFailure:
 484     raise

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_test.py:60:
super(TestNotSupportedOnPlatformFailure, self).__init__(message)
Constructors are inherited in Python, not needed here

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/test.py (right):

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/test.py:82: results =
page_test_results.PageTestResults()
Unused?

[ernstm, 2014-03-28 17:54:04]

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_runner.py:484: raise failure
On 2014/03/27 23:22:10, dtu wrote:
> Python shorthand:
>  483   except page_test.TestNotSupportedOnPlatformFailure:
>  484     raise

Done.

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_test.py:60:
super(TestNotSupportedOnPlatformFailure, self).__init__(message)
On 2014/03/27 23:22:10, dtu wrote:
> Constructors are inherited in Python, not needed here

Done.

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/test.py (right):

https://codereview.chromium.org/100443007/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/test.py:82: results =
page_test_results.PageTestResults()
On 2014/03/27 23:22:10, dtu wrote:
> Unused?

No, that is required. run_benchmark goes through that path. If page_runner.Run
throws an exception, results will be uninitialized and we get an error when
calling PrintSummary on it.

[ernstm, 2014-03-28 17:55:15]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100443007/130001

[commit-bot: I haz the power, 2014-03-28 19:24:24]

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

[commit-bot: I haz the power, 2014-03-28 19:24:25]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[ernstm, 2014-03-28 20:22:55]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-03-28 20:25:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100443007/150001

[commit-bot: I haz the power, 2014-03-28 23:44:45]

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

[commit-bot: I haz the power, 2014-03-28 23:44:46]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[ernstm, 2014-03-29 00:28:32]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-03-29 00:30:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100443007/150001

[commit-bot: I haz the power, 2014-03-29 02:04:12]

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

[commit-bot: I haz the power, 2014-03-29 02:04:12]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[ernstm, 2014-03-29 05:30:53]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-03-29 05:31:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100443007/150001

[commit-bot: I haz the power, 2014-03-29 07:06:01]

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

[commit-bot: I haz the power, 2014-03-29 07:06:02]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[ernstm, 2014-03-30 04:39:02]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-03-30 04:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100443007/150001

[commit-bot: I haz the power, 2014-03-30 05:59:38]

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

[commit-bot: I haz the power, 2014-03-30 05:59:38]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[ernstm, 2014-03-30 20:21:25]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-03-30 20:21:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100443007/150001

[commit-bot: I haz the power, 2014-03-31 00:43:59]

Change committed as 260475