Kill the page_runner.py --output (and -o) flag and use --output-dir instead.

This makes some sense, since as we potentially output more and more files
(instead of a single results.html), we want to store them all in the same
location.

SHERIFFS: If error "This flag is deprecated. Please use --output-dir instead."
arises, this patch can be safely reverted.

Original discussion at https://codereview.chromium.org/616063004/

BUG=

Review URL: https://codereview.chromium.org/651383002

Cr-Commit-Position: refs/heads/master@{#300309}

tools/telemetry/telemetry/page/page_runner.py

@@ -7,7 +7,6 @@ import optparse
 import os
 import random
 import sys
-import tempfile
 import time
 
 from telemetry import decorators
@@ -38,7 +37,6 @@ class _RunState(object):
     self._did_login_for_current_page = False
     self._current_page = None
     self._current_tab = None
-    self.profiler_dir = None
 
   def StartBrowserIfNeeded(self, test, page_set, page, possible_browser,
                           finder_options):
@@ -166,9 +164,7 @@ class _RunState(object):
       self._append_to_existing_wpr = True
 
   def StartProfiling(self, page, finder_options):
-    if not self.profiler_dir:
-      self.profiler_dir = tempfile.mkdtemp()
-    output_file = os.path.join(self.profiler_dir, page.file_safe_name)
+    output_file = os.path.join(finder_options.output_dir, page.file_safe_name)
     is_repeating = (finder_options.page_repeat != 1 or
                     finder_options.pageset_repeat != 1)
     if is_repeating:
@@ -220,6 +216,7 @@ def AddCommandLineArgs(parser):
 
 def ProcessCommandLineArgs(parser, args):
   page_filter.PageFilter.ProcessCommandLineArgs(parser, args)
+  results_options.ProcessCommandLineArgs(parser, args)
 
   # Page set options
   if args.pageset_shuffle_order_file and not args.pageset_shuffle:

tools/telemetry/telemetry/page/page_runner_unittest.py

@@ -6,6 +6,8 @@ import logging
 import os
 import tempfile
 import unittest
+import StringIO
+import sys
 
 from telemetry import benchmark
 from telemetry import decorators
@@ -284,36 +286,31 @@ class PageRunnerTests(unittest.TestCase):
         results.AddValue(scalar.ScalarValue(
             page, 'metric', 'unit', self.i))
 
-    output_file = tempfile.NamedTemporaryFile(delete=False).name
-    try:
-      options = options_for_unittests.GetCopy()
-      options.output_formats = ['buildbot']
-      options.output_file = output_file
-      options.suppress_gtest_report = True
-      options.reset_results = None
-      options.upload_results = None
-      options.results_label = None
+    options = options_for_unittests.GetCopy()
+    options.output_formats = ['buildbot']
+    options.suppress_gtest_report = True
+    options.reset_results = None
+    options.upload_results = None
+    options.results_label = None
+    options.page_repeat = 1
+    options.pageset_repeat = 2
+    SetUpPageRunnerArguments(options)
 
-      options.page_repeat = 1
-      options.pageset_repeat = 2
-      SetUpPageRunnerArguments(options)
+    output = StringIO.StringIO()
+    real_stdout = sys.stdout
+    sys.stdout = output
+    try:
       results = results_options.CreateResults(EmptyMetadataForTest(), options)
       page_runner.Run(Measurement(), ps, expectations, options, results)
       results.PrintSummary()
+      contents = output.getvalue()
       self.assertEquals(4, len(GetSuccessfulPageRuns(results)))
       self.assertEquals(0, len(results.failures))
-      with open(output_file) as f:
-        stdout = f.read()
-      self.assertIn('RESULT metric: blank.html= [1,3] unit', stdout)
-      self.assertIn('RESULT metric: green_rect.html= [2,4] unit', stdout)
-      self.assertIn('*RESULT metric: metric= [1,2,3,4] unit', stdout)
+      self.assertIn('RESULT metric: blank.html= [1,3] unit', contents)
+      self.assertIn('RESULT metric: green_rect.html= [2,4] unit', contents)
+      self.assertIn('*RESULT metric: metric= [1,2,3,4] unit', contents)
     finally:
-      # TODO(chrishenry): This is a HACK!!1 Really, the right way to
-      # do this is for page_runner (or output formatter) to close any
-      # files it has opened.
-      for formatter in results._output_formatters:  # pylint: disable=W0212
-        formatter.output_stream.close()
-      os.remove(output_file)
+      sys.stdout = real_stdout
 
   def testCredentialsWhenLoginFails(self):
     self.SuppressExceptionFormatting()

tools/telemetry/telemetry/page/profile_generator.py

@@ -110,9 +110,6 @@ def AddCommandLineArgs(parser):
       default=None,
       help='Type of profile to generate. '
            'Supported values: %s' % legal_profile_creators)
-  group.add_option('--output-dir',
-      dest='output_dir',
-      help='Generated profile is placed in this directory.')
   parser.add_option_group(group)
 
 

tools/telemetry/telemetry/results/results_options.py

@@ -18,10 +18,20 @@ from telemetry.results import page_test_results
 from telemetry.results import progress_reporter
 
 # Allowed output formats. The default is the first item in the list.
-_OUTPUT_FORMAT_CHOICES = ('html', 'buildbot', 'block', 'csv', 'gtest', 'json',
+_OUTPUT_FORMAT_CHOICES = ('html', 'buildbot', 'csv', 'gtest', 'json',
     'chartjson', 'csv-pivot-table', 'none')
 
 
+# Filenames to use for given output formats.
+_OUTPUT_FILENAME_LOOKUP = {
+    'html': 'results.html',
+    'csv': 'results.csv',
+    'json': 'results.json',
+    'chartjson': 'results-chart.json',
+    'csv-pivot-table': 'results-pivot-table.csv'
+}
+
+
 def AddResultsOptions(parser):
   group = optparse.OptionGroup(parser, 'Results options')
   group.add_option('--chartjson', action='store_true',
@@ -34,6 +44,8 @@ def AddResultsOptions(parser):
                     dest='output_file',
                     default=None,
                     help='Redirects output to a file. Defaults to stdout.')
+  group.add_option('--output-dir', default=util.GetBaseDir(),
+                    help='Where to save output data after the run.')
   group.add_option('--output-trace-tag',
                     default='',
                     help='Append a tag to the key of each result trace. Use '
@@ -51,17 +63,33 @@ def AddResultsOptions(parser):
   parser.add_option_group(group)
 
 
-def _GetOutputStream(output_format, output_file):
+def ProcessCommandLineArgs(parser, args):
+  # TODO(ariblue): Delete this flag entirely at some future data, when the
+  # existence of such a flag has been long forgotten.
+  if args.output_file:
+    parser.error('This flag is deprecated. Please use --output-dir instead.')
+
+  try:
+    os.makedirs(args.output_dir)
+  except OSError:
+    # Do nothing if the output directory already exists. Existing files will
+    # get overwritten.
+    pass
+
+  args.output_dir = os.path.expanduser(args.output_dir)
+
+
+def _GetOutputStream(output_format, output_dir):
   assert output_format in _OUTPUT_FORMAT_CHOICES, 'Must specify a valid format.'
   assert output_format not in ('gtest', 'none'), (
       'Cannot set stream for \'gtest\' or \'none\' output formats.')
 
-  if output_file is None:
-    if output_format != 'html' and output_format != 'json':
-      return sys.stdout
-    output_file = os.path.join(util.GetBaseDir(), 'results.' + output_format)
+  if output_format == 'buildbot':
+    return sys.stdout
 
-  output_file = os.path.expanduser(output_file)
+  assert output_format in _OUTPUT_FILENAME_LOOKUP, (
+      'No known filename for the \'%s\' output format' % output_format)
+  output_file = os.path.join(output_dir, _OUTPUT_FILENAME_LOOKUP[output_format])
   open(output_file, 'a').close()  # Create file if it doesn't exist.
   return open(output_file, 'r+')
 
@@ -82,20 +110,12 @@ def CreateResults(benchmark_metadata, options):
   if not options.output_formats:
     options.output_formats = [_OUTPUT_FORMAT_CHOICES[0]]
 
-  # TODO(chrishenry): It doesn't make sense to have a single output_file flag
-  # with multiple output formatters. We should explore other possible options:
-  #   - Have an output_file per output formatter
-  #   - Have --output-dir instead of --output-file
-  if len(options.output_formats) != 1 and options.output_file:
-    raise Exception('Cannot specify output_file flag with multiple output '
-                    'formats.')
-
   output_formatters = []
   for output_format in options.output_formats:
     if output_format == 'none' or output_format == "gtest" or options.chartjson:
       continue
 
-    output_stream = _GetOutputStream(output_format, options.output_file)
+    output_stream = _GetOutputStream(output_format, options.output_dir)
     if output_format == 'csv':
       output_formatters.append(csv_output_formatter.CsvOutputFormatter(
           output_stream))