Store files locally in a different location after they've been posted.

tools/testing/perf_testing/run_perf_tests.py

Index: tools/testing/perf_testing/run_perf_tests.py
===================================================================
--- tools/testing/perf_testing/run_perf_tests.py	(revision 7396)
+++ tools/testing/perf_testing/run_perf_tests.py	(working copy)
@@ -144,18 +144,19 @@
 
     Args:
       dir_name: the directory we will create if it does not exist."""
-    dir_path = os.path.join(DART_INSTALL_LOCATION, 'tools', 
-                            'testing', 'perf_testing', dir_name)
-    if not os.path.exists(dir_path):
-      os.mkdir(dir_path)
-      print 'Creating output directory ', dir_path
+    for directory in [dir_name, 'old', os.path.join('old', dir_name)]:
+      dir_path = os.path.join(DART_INSTALL_LOCATION, 'tools', 
+                              'testing', 'perf_testing', directory)
+      if not os.path.exists(dir_path):
+        os.mkdir(dir_path)

[vsm, 2012/05/07 20:25:54]

You can eliminate 'old' from your list and just call makedirs (recursive dir
create) here.  Consider unrolling the loop.  I think a local function is a
cleaner way to factor out redundancy.  E.g.,

make_if_necessary(dir_name)
make_if_necessary(os.path.join('old', dir_name)

[Emily Fortuna, 2012/05/08 01:24:51]

Done.

+        print 'Creating output directory ', dir_path
 
   def has_new_code(self):
     """Tests if there are any newer versions of files on the server."""
     os.chdir(DART_INSTALL_LOCATION)
     # Pass 'p' in if we have a new certificate for the svn server, we want to
     # (p)ermanently accept it.
-    results = self.run_cmd(['svn', 'st', '-u'], std_in='p')
+    results = self.run_cmd(['svn', 'st', '-u'], std_in='p\r\n')
     for line in results:
       if '*' in line:
         return True
@@ -351,16 +352,21 @@
 
     os.chdir(os.path.join('tools', 'testing', 'perf_testing'))
 
-    files = os.listdir(self.result_folder_name)
-    for afile in files:
-      if not afile.startswith('.'):
-        self.file_processor.process_file(afile)
+    for directory in [os.path.join('old', self.result_folder_name),
+                      self.result_folder_name]:

[vsm, 2012/05/07 20:25:54]

I would unroll this loop for clarity also - especially given the if old_dir / if
not old_dir in the body.

[Emily Fortuna, 2012/05/08 01:24:51]

Done.

+      old_dir = '/old/' in directory or '\\old\\' in directory
+      files = os.listdir(directory)
+      for afile in files:
+        if not afile.startswith('.'):
+          if self.file_processor.process_file(afile, not old_dir) and \

[vsm, 2012/05/07 20:25:54]

I'd take process_file out of the conditional.  It's easy to miss side-effecting
code in there.

[Emily Fortuna, 2012/05/08 01:24:51]

Done.

+              not old_dir:
+            shutil.move(os.path.join(self.result_folder_name, afile),
+                        os.path.join('old', self.result_folder_name, afile))
+      if 'plt' in globals() and old_dir:
+        # Only run Matplotlib if it is installed.
+        self.grapher.plot_results('%s.png' % self.result_folder_name)
 
-    if 'plt' in globals():
-      # Only run Matplotlib if it is installed.
-      self.grapher.plot_results('%s.png' % self.result_folder_name)
 
-
 class Tester(object):
   """The base level visitor class that runs tests. It contains convenience 
   methods that many Tester objects use. Any class that would like to be a
@@ -418,14 +424,6 @@
     """Perform any initial setup required before the test is run."""
     pass
 
-  def should_report_results(self, afile):
-    """We store all trace files locally, but we don't want to post all of the
-    results every time, so we only attempt to post results for recent runs."""
-    cur_time = time.time()
-    file_mod_time = os.path.getmtime(os.path.join(
-        self.test.result_folder_name, afile))
-    return cur_time - file_mod_time < 1000 # Files modified in the last ~15 min.
-
   def report_results(self, benchmark_name, score, platform, variant, 
                      revision_number, metric):
     """Store the results of the benchmark run.
@@ -681,7 +679,7 @@
               append=True)
 
   class CommonBrowserFileProcessor(Processor):
-    def process_file(self, afile):
+    def process_file(self, afile, should_post_file):
       """Comb through the html to find the performance results.
       Returns: True if we successfully posted our data to storage and/or we can
           delete the trace file."""
@@ -726,8 +724,7 @@
         bench_dict = self.test.values_dict[browser][version]
         bench_dict[name] += [float(score)]
         self.test.revision_dict[browser][version][name] += [revision_num]
-        if self.should_report_results(afile) and \
-            not self.test.test_runner.no_upload:
+        if not self.test.test_runner.no_upload and should_post_file:
           upload_success = upload_success and self.report_results(
               name, score, browser, version, revision_num, self.SCORE)
         else:
@@ -870,7 +867,7 @@
 
 
   class DromaeoFileProcessor(Processor):
-    def process_file(self, afile):
+    def process_file(self, afile, should_post_file):
       """Comb through the html to find the performance results.
       Returns: True if we successfully posted our data to storage."""
       parts = afile.split('-')
@@ -906,8 +903,7 @@
                 bench_dict[name] += [float(score)]
                 self.test.revision_dict[browser][version][name] += \
                     [revision_num]
-                if self.should_report_results(afile) and \
-                    not self.test.test_runner.no_upload:
+                if not self.test.test_runner.no_upload and should_post_file:
                   upload_success = upload_success and self.report_results(
                       name, score, browser, version, revision_num, self.SCORE)
                 else:
@@ -1000,7 +996,7 @@
 
 
   class DromaeoSizeProcessor(Processor):
-    def process_file(self, afile):
+    def process_file(self, afile, should_post_file):
       """Pull all the relevant information out of a given tracefile.
 
       Args:
@@ -1033,8 +1029,7 @@
           self.test.values_dict['commandline'][variant][metric] += [num]
           self.test.revision_dict['commandline'][variant][metric] += \
               [revision_num]
-          if self.should_report_results(afile) and \
-              not self.test.test_runner.no_upload:
+          if not self.test.test_runner.no_upload and should_post_file:
             upload_success = upload_success and self.report_results(
                 metric, num, 'commandline', variant, revision_num,
                 self.CODE_SIZE)
@@ -1130,7 +1125,7 @@
 
   class CompileProcessor(Processor):
 
-    def process_file(self, afile):
+    def process_file(self, afile, should_post_file):
       """Pull all the relevant information out of a given tracefile.
 
       Args:
@@ -1160,8 +1155,7 @@
               score_type = self.CODE_SIZE
               if 'Compiling' in metric or 'Bootstrapping' in metric:
                 score_type = self.COMPILE_TIME
-              if self.should_report_results(afile) and \
-                  not self.test.test_runner.no_upload:
+              if not self.test.test_runner.no_upload and should_post_file:
                 if num < self.test.failure_threshold[metric]:
                   num = 0
                 upload_success = upload_success and self.report_results(