Added support for versioning of layout test results of run-webkit-tests runs

Tools/Scripts/webkitpy/layout_tests/controllers/manager.py

Index: Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py b/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
index 87cbe17aab78d8e30a7bb83e97d36014c8aab680..01894d0583259e17cf3483bd354aeaf2e703aa07 100644
--- a/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
+++ b/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
@@ -77,6 +77,9 @@ class Manager(object):
         self._options = options
         self._printer = printer
         self._expectations = None
+        # Actions related to archiving of the results.
+        self.ARCHIVED_PATH = None

[Dirk Pranke, 2014/07/15 20:50:00]

ARCHIVED_PATH isn't a constant; don't use upper case for it. It also looks like
it isn't actually used at all ...

REVISION_INFO doesn't look like it is used at all.

[patro, 2014/07/16 14:37:36]

Done.

+        self.REVISION_INFO = ''
 
         self.HTTP_SUBDIR = 'http' + port.TEST_PATH_SEPARATOR
         self.PERF_SUBDIR = 'perf'
@@ -151,6 +154,23 @@ class Manager(object):
     def needs_servers(self, test_names):
         return any(self._test_requires_lock(test_name) for test_name in test_names)
 
+    def _rename_results_folder_if_required(self):
+        try:
+            timestamp = time.strftime("%Y-%m-%d-%H-%M-%S", time.gmtime(self._filesystem.mtime(self._filesystem.join(self._results_directory, "results.html"))))
+            archived_name = ''.join((self._filesystem.basename(self._results_directory), "_", timestamp))
+            archived_path = self._filesystem.join(self._filesystem.dirname(self._results_directory), archived_name)
+            self._filesystem.move(self._results_directory, archived_path)
+            return archived_path
+        except OSError, e:
+            # It might be possible that results.html was not generated in previous run, because the test
+            # run was interrupted even before testing started. In those cases, don't archive the folder.
+            # Simply override the current folder contents with new results.
+            import errno
+            if e.errno == errno.EEXIST:
+                print "Found no result.html in the folder, ignoring this result and overwritten the folder..."
+                _log.info("Found no result.html in the folder, ignoring this result and overwritten the folder...")

[Dirk Pranke, 2014/07/15 20:50:00]

Don't use both a print and a _log.info(). Just use log.warning() (which is the
appropriate thing for this).

I would also rewrite this as 'No results.html file found in previous run,
skipping it.'.

Put the try/except around just line 159, to be clear about what you're actually
checking. Right now you might get an OSError from the filesystem.move() call,
too, and you'll print the wrong warning for it.

[patro, 2014/07/16 14:37:36]

Done.

+                return None
+
     def _set_up_run(self, test_names):
         self._printer.write_update("Checking build ...")
         if self._options.build:
@@ -176,6 +196,11 @@ class Manager(object):
         if self._options.clobber_old_results:
             self._clobber_old_results()
 
+        # FIXME:: Here the condition has to be (archive_prev_results) and (total_archived_results < MAX_ARCHIVE_RESULTS)

[Dirk Pranke, 2014/07/15 20:50:00]

This comment is referring to things that don't exist. However, it seems to be
getting at the idea that we don't want to archive an indefinite number of
results, which seems like a good idea? Maybe rewrite the FIXME to be clearer,
though I would just implement this logic as part of this patch.

[patro, 2014/07/16 14:37:36]

Done.

+        if self._options.enable_versioned_results and self._filesystem.exists(self._results_directory):
+            # Rename the existing results folder for archiving.
+            self.ARCHIVED_PATH = self._rename_results_folder_if_required()

[Dirk Pranke, 2014/07/15 20:50:00]

the actual method is renaming the folder unconditionally, so I would just call
it 'self._rename_old_results_folder' (no "_if_required").

Also, if someone passes --clobber-old-results and --enable-versioned-results,
you'll clobber the data and then rename it; probably not what we want. 

I would probably either make the two flags mutually exclusive (and error out
when you parse the arguments), or make enable-versioned-results override
--clobber and log a warning to say that that's what you're doing (I think I like
the latter option better).

[patro, 2014/07/16 14:37:36]

Making the flags --enable-versioned-results --clobber-old-results mutually
exclusive

+
         # Create the output directory if it doesn't already exist.
         self._port.host.filesystem.maybe_make_directory(self._results_directory)