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

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

 # Copyright (C) 2010 Google Inc. All rights reserved.
 # Copyright (C) 2010 Gabor Rapcsanyi (rgabor@inf.u-szeged.hu), University of Szeged
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
@@ skipping 59 matching lines @@
         Args:
           port: an object implementing port-specific
           options: a dictionary of command line options
           printer: a Printer object to record updates to.
         """
         self._port = port
         self._filesystem = port.host.filesystem
         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'
         self.WEBSOCKET_SUBDIR = 'websocket' + port.TEST_PATH_SEPARATOR
         self.LAYOUT_TESTS_DIRECTORY = 'LayoutTests'
         self._http_server_started = False
         self._websockets_server_started = False
 
         self._results_directory = self._port.results_directory()
         self._finder = LayoutTestFinder(self._port, self._options)
@@ skipping 54 matching lines @@
     def _test_is_expected_missing(self, test_file):
         expectations = self._expectations.model().get_expectations(test_file)
         return test_expectations.MISSING in expectations or test_expectations.NEEDS_REBASELINE in expectations or test_expectations.NEEDS_MANUAL_REBASELINE in expectations
 
     def _test_is_slow(self, test_file):
         return test_expectations.SLOW in self._expectations.model().get_expectations(test_file)
 
     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:
             exit_code = self._port.check_build(self.needs_servers(test_names), self._printer)
             if exit_code:
                 _log.error("Build check failed")
                 return exit_code
 
         # This must be started before we check the system dependencies,
         # since the helper may do things to make the setup correct.
         if self._options.pixel_tests:
             self._printer.write_update("Starting pixel test helper ...")
             self._port.start_helper()
 
         # Check that the system dependencies (themes, fonts, ...) are correct.
         if not self._options.nocheck_sys_deps:
             self._printer.write_update("Checking system dependencies ...")
             exit_code = self._port.check_sys_deps(self.needs_servers(test_names))
             if exit_code:
                 self._port.stop_helper()
                 return exit_code
 
         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)
 
         self._port.setup_test_run()
         return test_run_results.OK_EXIT_STATUS
 
     def run(self, args):
         """Run the tests and return a RunDetails object with the results."""
         start_time = time.time()
         self._printer.write_update("Collecting tests ...")
@@ skipping 248 matching lines @@
             return int(worker_name.split('/')[1]) if worker_name else -1
 
         stats = {}
         for result in initial_results.results_by_name.values():
             if result.type != test_expectations.SKIP:
                 stats[result.test_name] = {'results': (_worker_number(result.worker_name), result.test_number, result.pid, int(result.test_run_time * 1000), int(result.total_run_time * 1000))}
         stats_trie = {}
         for name, value in stats.iteritems():
             json_results_generator.add_path_to_trie(name, value, stats_trie)
         return stats_trie