For consistency with benchmark, rename unittest.output_formatter -> progress_reporter.

For consistency with benchmark, rename unittest.output_formatter -> progress_reporter.

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

[nednguyen, 2014-08-04 04:57:58]

On 2014/08/02 20:56:16, chrishenry wrote:

LGTM

You would need to change run_unittests as well:
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu/r...

+kbr

[nednguyen, 2014-08-04 04:58:09]

https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
File tools/telemetry/telemetry/unittest/run_tests.py (right):

https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
tools/telemetry/telemetry/unittest/run_tests.py:21: self._progress_reporters =
tuple(progress_reporters)
It looks like we would want to avoid these reporters to share a same output
stream. Would an assertion be reasonable here?

[chrishenry, 2014-08-04 16:59:08]

On 2014/08/04 04:57:58, nednguyen wrote:
> On 2014/08/02 20:56:16, chrishenry wrote:
> 
> LGTM
> 
> You would need to change run_unittests as well:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu/r...
> 
> +kbr

Good catch! Done.

[chrishenry, 2014-08-04 16:59:41]

https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
File tools/telemetry/telemetry/unittest/run_tests.py (right):

https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
tools/telemetry/telemetry/unittest/run_tests.py:21: self._progress_reporters =
tuple(progress_reporters)
On 2014/08/04 04:58:09, nednguyen wrote:
> It looks like we would want to avoid these reporters to share a same output
> stream. Would an assertion be reasonable here?

Why not? In fact, I'd imagine that all of them would want to write to
sys.stdout, no?

[nednguyen, 2014-08-04 17:05:47]

On 2014/08/04 16:59:41, chrishenry wrote:
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> File tools/telemetry/telemetry/unittest/run_tests.py (right):
> 
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> tools/telemetry/telemetry/unittest/run_tests.py:21: self._progress_reporters =
> tuple(progress_reporters)
> On 2014/08/04 04:58:09, nednguyen wrote:
> > It looks like we would want to avoid these reporters to share a same output
> > stream. Would an assertion be reasonable here?
> 
> Why not? In fact, I'd imagine that all of them would want to write to
> sys.stdout, no?

I think the output could be weird and unpredictable if we allow multiple
progress reporters to the same stream. In practice, will we ever have more than
1 progress reporters output to stdout at once?

[chrishenry, 2014-08-04 17:11:01]

On 2014/08/04 17:05:47, nednguyen wrote:
> On 2014/08/04 16:59:41, chrishenry wrote:
> >
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> > File tools/telemetry/telemetry/unittest/run_tests.py (right):
> > 
> >
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> > tools/telemetry/telemetry/unittest/run_tests.py:21: self._progress_reporters
=
> > tuple(progress_reporters)
> > On 2014/08/04 04:58:09, nednguyen wrote:
> > > It looks like we would want to avoid these reporters to share a same
output
> > > stream. Would an assertion be reasonable here?
> > 
> > Why not? In fact, I'd imagine that all of them would want to write to
> > sys.stdout, no?
> 
> I think the output could be weird and unpredictable if we allow multiple
> progress reporters to the same stream. In practice, will we ever have more
than
> 1 progress reporters output to stdout at once?

My thought is no. In the benchmark case, progress_reporter is a single instance,
not a list.

However, the unittest version does not have a separate OutputFormatter
interface, unlike the benchmark.

I think it's actually ok. Users should never care about setting the progress
reporter here, in fact there is no flag that allow them to do so. So if someone
add multiple progress reporter, hopefully they (including the code reviewer) had
thought through the implication of writing to the same output stream, if they in
fact do.

[nednguyen, 2014-08-04 17:26:22]

On 2014/08/04 17:11:01, chrishenry wrote:
> On 2014/08/04 17:05:47, nednguyen wrote:
> > On 2014/08/04 16:59:41, chrishenry wrote:
> > >
> >
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> > > File tools/telemetry/telemetry/unittest/run_tests.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> > > tools/telemetry/telemetry/unittest/run_tests.py:21:
self._progress_reporters
> =
> > > tuple(progress_reporters)
> > > On 2014/08/04 04:58:09, nednguyen wrote:
> > > > It looks like we would want to avoid these reporters to share a same
> output
> > > > stream. Would an assertion be reasonable here?
> > > 
> > > Why not? In fact, I'd imagine that all of them would want to write to
> > > sys.stdout, no?
> > 
> > I think the output could be weird and unpredictable if we allow multiple
> > progress reporters to the same stream. In practice, will we ever have more
> than
> > 1 progress reporters output to stdout at once?
> 
> My thought is no. In the benchmark case, progress_reporter is a single
instance,
> not a list.
> 
> However, the unittest version does not have a separate OutputFormatter
> interface, unlike the benchmark.

Gotcha. How about just keep the OutputFormatter files and Dirk can just subclass
his json output formatter with it? 

> 
> I think it's actually ok. Users should never care about setting the progress
> reporter here, in fact there is no flag that allow them to do so. So if
someone
> add multiple progress reporter, hopefully they (including the code reviewer)
had
> thought through the implication of writing to the same output stream, if they
in
> fact do.

[chrishenry, 2014-08-04 17:43:36]

On 2014/08/04 17:26:22, nednguyen wrote:
> On 2014/08/04 17:11:01, chrishenry wrote:
> > On 2014/08/04 17:05:47, nednguyen wrote:
> > > On 2014/08/04 16:59:41, chrishenry wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> > > > File tools/telemetry/telemetry/unittest/run_tests.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/439503006/diff/1/tools/telemetry/telemetry/un...
> > > > tools/telemetry/telemetry/unittest/run_tests.py:21:
> self._progress_reporters
> > =
> > > > tuple(progress_reporters)
> > > > On 2014/08/04 04:58:09, nednguyen wrote:
> > > > > It looks like we would want to avoid these reporters to share a same
> > output
> > > > > stream. Would an assertion be reasonable here?
> > > > 
> > > > Why not? In fact, I'd imagine that all of them would want to write to
> > > > sys.stdout, no?
> > > 
> > > I think the output could be weird and unpredictable if we allow multiple
> > > progress reporters to the same stream. In practice, will we ever have more
> > than
> > > 1 progress reporters output to stdout at once?
> > 
> > My thought is no. In the benchmark case, progress_reporter is a single
> instance,
> > not a list.
> > 
> > However, the unittest version does not have a separate OutputFormatter
> > interface, unlike the benchmark.
> 
> Gotcha. How about just keep the OutputFormatter files and Dirk can just
subclass
> his json output formatter with it? 

Chatted offline. The OutputFormatter refactoring is much more major than we're
willing to take in right now. The goal of this patch is simply to reduce
confusion from naming, since OutputFormatter in benchmark now means something
different than the OutputFormatter here. Certainly a ProgressReporter is a
superset of OutputFormatter, so the unittest framework isn't losing any
capability by not having a separate OutputFormatter. I'll leave this to
dtu/dpranke to decide on whether they want to create a separate OutputFormatter
interface, just like in benchmark.

[Ken Russell (switch to Gerrit), 2014-08-04 18:10:25]

content/test/gpu LGTM

As mentioned before, I think the concepts of output formatters and progress
reporters could profitably be unified. I agree there's not much point in having
multiple progress reporters, but using the same interface for both, and being
able to add instances of both to the reporting chain, seems like a nice,
symmetric cleanup to me.

[chrishenry, 2014-08-04 23:07:31]

+bolian for chrome_proxy. Thanks!!

[chrishenry, 2014-08-08 00:18:30]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-08-08 00:54:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/439503006/100001

[bolian, 2014-08-08 03:54:23]

lgtm

[commit-bot: I haz the power, 2014-08-08 09:52:47]

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

[commit-bot: I haz the power, 2014-08-08 09:52:48]

Failed to apply patch for tools/telemetry/telemetry/unittest/run_tests.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/telemetry/telemetry/unittest/run_tests.py
  Hunk #4 FAILED at 150.
  1 out of 4 hunks FAILED -- saving rejects to file
tools/telemetry/telemetry/unittest/run_tests.py.rej

Patch:       tools/telemetry/telemetry/unittest/run_tests.py
Index: tools/telemetry/telemetry/unittest/run_tests.py
diff --git a/tools/telemetry/telemetry/unittest/run_tests.py
b/tools/telemetry/telemetry/unittest/run_tests.py
index
bc36e2b0653b524d89c4cfddd0663963123ef92f..4f37bc0e50281f02d38b0dd36face08b03c78468
100644
--- a/tools/telemetry/telemetry/unittest/run_tests.py
+++ b/tools/telemetry/telemetry/unittest/run_tests.py
@@ -11,14 +11,14 @@ from telemetry.core import browser_options
 from telemetry.core import command_line
 from telemetry.core import discover
 from telemetry.unittest import json_results
-from telemetry.unittest import output_formatter
+from telemetry.unittest import progress_reporter
 
 
 class Config(object):
-  def __init__(self, top_level_dir, test_dirs, output_formatters):
+  def __init__(self, top_level_dir, test_dirs, progress_reporters):
     self._top_level_dir = top_level_dir
     self._test_dirs = tuple(test_dirs)
-    self._output_formatters = tuple(output_formatters)
+    self._progress_reporters = tuple(progress_reporters)
 
   @property
   def top_level_dir(self):
@@ -29,13 +29,13 @@ class Config(object):
     return self._test_dirs
 
   @property
-  def output_formatters(self):
-    return self._output_formatters
+  def progress_reporters(self):
+    return self._progress_reporters
 
 
 def Discover(start_dir, top_level_dir=None, pattern='test*.py'):
   loader = unittest.defaultTestLoader
-  loader.suiteClass = output_formatter.TestSuite
+  loader.suiteClass = progress_reporter.TestSuite
 
   test_suites = []
   modules = discover.DiscoverModules(start_dir, top_level_dir, pattern)
@@ -82,7 +82,7 @@ def DiscoverTests(search_dirs, top_level_dir,
possible_browser,
     method = getattr(test, test._testMethodName)
     return decorators.IsEnabled(method, possible_browser)
 
-  wrapper_suite = output_formatter.TestSuite()
+  wrapper_suite = progress_reporter.TestSuite()
   for search_dir in search_dirs:
     wrapper_suite.addTests(Discover(search_dir, top_level_dir,
'*_unittest.py'))
   return FilterSuite(wrapper_suite, IsTestSelected)
@@ -150,9 +150,9 @@ class RunTestsCommand(command_line.OptparseCommand):
     test_suite = DiscoverTests(
         config.test_dirs, config.top_level_dir, possible_browser,
         args.positional_args, args.run_disabled_tests)
-    runner = output_formatter.TestRunner()
+    runner = progress_reporter.TestRunner()
     result = runner.run(
-        test_suite, config.output_formatters, args.repeat_count, args)
+        test_suite, config.progress_reporters, args.repeat_count, args)
 
     json_results.WriteandUploadResultsIfNecessary(args, test_suite, result)

[chrishenry, 2014-08-09 06:16:43]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-08-09 06:17:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/439503006/120001

[commit-bot: I haz the power, 2014-08-09 08:29:23]

Change committed as 288541

[ilja, 2014-08-11 18:22:09]

A revert of this CL has been created in
https://codereview.chromium.org/459943002/ by ihf@chromium.org.

The reason for reverting is: Reverting due to ChromeOS break, see
https://code.google.com/p/chromium/issues/detail?id=402451#c4.