[telemetry] OutputFormatter for unit tests.

[telemetry] OutputFormatter for unit tests.


BUG=346956
TEST=tools/telemetry/run_tests && tools/perf/run_tests && content/test/gpu/run_unittests
R=kbr@chromium.org, nednguyen@google.com, tonyg@chromium.org

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

[tonyg, 2014-07-09 16:58:10]

Very nice!

lgtm in advance of comments.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/gtest_output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_output_formatter.py:20: class
GTestOutputFormatter(output_formatter.OutputFormatter):
It would be nice to have class level documentation here. In particular, it
should describe any build bot dependencies and pointers to the documentation for
the format (if available).

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py
(left):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py:108: '[ 
FAILED  ]  gtest_unittest_results_unittest.TestBar.runTezt\n\n'
It looks to me like we lost some coverage. Can we bring it back?

[nednguyen, 2014-07-09 17:26:46]

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/run_tests
File tools/telemetry/run_tests (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/run_test...
tools/telemetry/run_tests:18: base_dir, [base_dir], output_formatters)
It seems weird for output_formatters to be part of run_test.environment. What
about "run_test.output_formatters = ..." instead?

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:6: class
OutputFormatter(object):
Do we have any other classes besides gtest_output_formatter that will subclass
this?

The API of this OutputFormatter seems to support python unittest, not telemetry
benchmark.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter_test_runner.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter_test_runner.py:9: 
I don't see why we want to add prefix OutputFormatter- to classes' names in this
file.

[Ken Russell (switch to Gerrit), 2014-07-09 20:20:20]

LGTM overall. Nice work. Few comments.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/gtest_output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_output_formatter.py:65: '%d %s, listed
below:' % (len(all_errors), unit))
Can self._Print() not be used for these statements? It's weird to sidestep it
and then call it at the end of the method to flush the output stream.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py
(left):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py:108: '[ 
FAILED  ]  gtest_unittest_results_unittest.TestBar.runTezt\n\n'
On 2014/07/09 16:58:10, tonyg wrote:
> It looks to me like we lost some coverage. Can we bring it back?

Agreed. Please add a new test to gtest_output_formatter_unittest.py which fails
a test and also calls StopTestRun() against the formatter.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter_test_runner.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter_test_runner.py:9: 
On 2014/07/09 17:26:46, nednguyen wrote:
> I don't see why we want to add prefix OutputFormatter- to classes' names in
this
> file.

Agree that OutputFormatterTestSuite is a mouthful.

[dtu, 2014-07-10 00:35:02]

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/run_tests
File tools/telemetry/run_tests (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/run_test...
tools/telemetry/run_tests:18: base_dir, [base_dir], output_formatters)
On 2014/07/09 17:26:46, nednguyen wrote:
> It seems weird for output_formatters to be part of run_test.environment. What
> about "run_test.output_formatters = ..." instead?

I don't think it's weird. Different callers are going to want output in
different formats, and environment is there to provide customization at this
level. E.g. ChromeOS won't switch to JSON at the same time as chromium.perf.
We'd like to have fewer globals - it's on my plate to move environment into a
variable that's passed to main().

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/gtest_output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_output_formatter.py:20: class
GTestOutputFormatter(output_formatter.OutputFormatter):
On 2014/07/09 16:58:10, tonyg wrote:
> It would be nice to have class level documentation here. In particular, it
> should describe any build bot dependencies and pointers to the documentation
for
> the format (if available).

? There are none of either of these.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_output_formatter.py:65: '%d %s, listed
below:' % (len(all_errors), unit))
On 2014/07/09 20:20:20, Ken Russell wrote:
> Can self._Print() not be used for these statements? It's weird to sidestep it
> and then call it at the end of the method to flush the output stream.

Done.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py
(left):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py:108: '[ 
FAILED  ]  gtest_unittest_results_unittest.TestBar.runTezt\n\n'
On 2014/07/09 16:58:10, tonyg wrote:
> It looks to me like we lost some coverage. Can we bring it back?

Done.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/gtest_unittest_results_unittest.py:108: '[ 
FAILED  ]  gtest_unittest_results_unittest.TestBar.runTezt\n\n'
On 2014/07/09 20:20:20, Ken Russell wrote:
> On 2014/07/09 16:58:10, tonyg wrote:
> > It looks to me like we lost some coverage. Can we bring it back?
> 
> Agreed. Please add a new test to gtest_output_formatter_unittest.py which
fails
> a test and also calls StopTestRun() against the formatter.

Done.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:6: class
OutputFormatter(object):
On 2014/07/09 17:26:46, nednguyen wrote:
> Do we have any other classes besides gtest_output_formatter that will subclass
> this?
> 
> The API of this OutputFormatter seems to support python unittest, not
telemetry
> benchmark.

Benchmarks only use one hook - PrintSummary(result), which is called
StopTestRun() here.
This API is not that different from the current PageTestResults API, which is
currently used by the GPU tests/benchmarks.
We will also subclass this to provide JSON output for unit tests, as requested
by the infra team (not related to benchmark JSON)
I wouldn't be opposed if we ended up using separate OutputFormatter base classes
for benchmarks and unit tests.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter_test_runner.py (right):

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter_test_runner.py:9: 
On 2014/07/09 20:20:20, Ken Russell wrote:
> On 2014/07/09 17:26:46, nednguyen wrote:
> > I don't see why we want to add prefix OutputFormatter- to classes' names in
> this
> > file.
> 
> Agree that OutputFormatterTestSuite is a mouthful.

Done.

https://codereview.chromium.org/382433002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter_test_runner.py:9: 
On 2014/07/09 17:26:46, nednguyen wrote:
> I don't see why we want to add prefix OutputFormatter- to classes' names in
this
> file.

Done. I moved everything from here into output_formatter.py so they can be
referenced as output_formatter.TestSuite, etc.

[nednguyen, 2014-07-10 00:51:20]

Can we rename Environment to s.t like UnittestConfig?

[nednguyen, 2014-07-10 00:51:28]

lgtm

[dtu, 2014-07-10 18:10:35]

On 2014/07/10 00:51:20, nednguyen wrote:
> Can we rename Environment to s.t like UnittestConfig?

Done.

[dtu, 2014-07-10 18:11:34]

I'm going to submit a change on the ChromeOS side for review so I can commit
them simultaneously.

[chrishenry, 2014-07-10 22:05:26]

I'm a bit wary about this change. I had thought that an OutputFormatter would be
state-based instead of command-based, in that it would eventually receive the
PageRun instances and format them appropriately. I think a state-based
OutputFormatter is easier to reason about. Most PageMeasurementResults
subclasses (except GTest) is already state-based anyway, in that they implement
PrintSummary only.

However, I understand that for unittest we do want the streaming behavior,
necessitating a command-based approach.

Interestingly, PageTestResults is already command-based (and hence
GTestTestResults). So one way to actually handle these difference is to not use
OutputFormatter for unittest at all and stick with GTestTestResults. In this
world:

* PageTestResults -> build a state tree (PageRuns) and pass them to
OutputFormatter, which will be purely state-based.
* GTestTestResults -> streams results directly.

(GTestTestResults isn't necessarily a PageTestResults subclass anymore.)

With this distinction, it's also nice in that we don't duplicate the streaming
pattern of PageTestResults in OutputFormatter.

Ned pointed that there might be a need for other streaming results (that is not
GTestTestResults). If this is indeed the case (we couldn't actually come up with
a good example of this), then maybe we should have 2 OutputFormatter interface,
one streaming and another state-based. I don't think we should do this now tho.

Thoughts?

[chrishenry, 2014-07-10 22:09:26]

On 2014/07/10 22:05:26, chrishenry wrote:
> I'm a bit wary about this change. I had thought that an OutputFormatter would
be
> state-based instead of command-based, in that it would eventually receive the
> PageRun instances and format them appropriately. I think a state-based
> OutputFormatter is easier to reason about. Most PageMeasurementResults
> subclasses (except GTest) is already state-based anyway, in that they
implement
> PrintSummary only.
> 
> However, I understand that for unittest we do want the streaming behavior,
> necessitating a command-based approach.
> 
> Interestingly, PageTestResults is already command-based (and hence
> GTestTestResults). So one way to actually handle these difference is to not
use
> OutputFormatter for unittest at all and stick with GTestTestResults. In this
> world:
> 
> * PageTestResults -> build a state tree (PageRuns) and pass them to
> OutputFormatter, which will be purely state-based.
> * GTestTestResults -> streams results directly.
> 
> (GTestTestResults isn't necessarily a PageTestResults subclass anymore.)
> 
> With this distinction, it's also nice in that we don't duplicate the streaming
> pattern of PageTestResults in OutputFormatter.
> 
> Ned pointed that there might be a need for other streaming results (that is
not
> GTestTestResults). If this is indeed the case (we couldn't actually come up
with
> a good example of this), then maybe we should have 2 OutputFormatter
interface,
> one streaming and another state-based. I don't think we should do this now
tho.
> 
> Thoughts?

Oh, then again, your OutputFormatter is actually in unittest directory. Is the
intention to have a separate OutputFormatter interface for benchmark? If so,
then this is inline with the above (again, I'm not sure we should unnecessarily
produce an interface for it though, unless we can think of another good
streaming case).

[chrishenry, 2014-07-10 22:13:11]

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:11: class
OutputFormatter(object):
Can you rename this to UnittestOutputFormatter/StreamingOutputFormatter and add
class doc?

[nduca, 2014-07-10 23:03:42]

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:15: def StartTest(self,
test):
Please add will and did conventions on here, depending on the timing of command
being reported. E.g. is the test running when this called, or is it about to?

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:34: pass
same here, will or did.

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:39: def Success(self,
test):
is this called once? what is the timing relative to StopTest? please clarify.

[nduca, 2014-07-10 23:05:16]

also please dont use this design for page output formatter. lets get dtu, nduca
and ned together to about design for the page output formatter when work is
ready to begin on it.

[dtu, 2014-07-11 20:04:42]

Sounds like the consensus is that this is okay for unit tests, but we won't use
it for benchmarks. That sounds good to me.

I had also envisioned an OutputFormatter that logging.info's the start and stop
of each test/page. Right now we do it in page_runner.py, which is ad hoc and is
redundant if you already have GTest output. (See: [1])

Unfortunately, the distinction between stream and stateful/PrintSummary() is not
the same as that between unittest and benchmark. For unit tests, we'd want GTest
in a stream, plus JSON as a summary.


[1]
http://build.chromium.org/p/chromium.gpu/builders/Win7%20Release%20%28NVIDIA%...

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/output_formatter.py (right):

https://codereview.chromium.org/382433002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/output_formatter.py:15: def StartTest(self,
test):
On 2014/07/10 23:03:41, nduca wrote:
> Please add will and did conventions on here, depending on the timing of
command
> being reported. E.g. is the test running when this called, or is it about to?

Are you sure? These hooks/names parallel those in unittest.TestResult and the
timing is dependent on Python's implementation.
https://docs.python.org/2/library/unittest.html#unittest.TestResult

[dtu, 2014-07-14 18:44:23]

The CQ bit was checked by dtu@chromium.org

[commit-bot: I haz the power, 2014-07-14 18:46:11]

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

[dtu, 2014-07-14 22:38:56]

The CQ bit was unchecked by dtu@chromium.org

[dtu, 2014-07-17 22:28:16]

Committed patchset #4 manually as r283907 (presubmit successful).