Add Validation for BuildFactory Configuration

master/skia_master_scripts/factory.py

Index: master/skia_master_scripts/factory.py
===================================================================
--- master/skia_master_scripts/factory.py	(revision 8776)
+++ master/skia_master_scripts/factory.py	(working copy)
@@ -16,7 +16,9 @@
 import config
 import config_private
 import ntpath
+import os
 import posixpath
+import utils
 
 
 # TODO(epoger): My intent is to make the build steps identical on all platforms
@@ -113,11 +115,11 @@
     self._do_patch_step = do_patch_step
 
     if not environment_variables:
-      my_env_vars = {}
+      self._env_vars = {}

[borenet, 2013/04/25 15:50:28]

Adding this to self so that it gets recorded.

     else:
-      my_env_vars = dict(environment_variables)
-    gyp_defines = my_env_vars.get('GYP_DEFINES', '')
-    my_env_vars['GYP_DEFINES'] = gyp_defines + \
+      self._env_vars = dict(environment_variables)
+    gyp_defines = self._env_vars.get('GYP_DEFINES', '')
+    self._env_vars['GYP_DEFINES'] = gyp_defines + \
         ' skia_warnings_as_errors=%d' % int(compile_warnings_as_errors)
 
     # Get an implementation of SkiaCommands as appropriate for
@@ -127,7 +129,7 @@
         target_platform=target_platform, factory=self,
         configuration=configuration, workdir=workdir,
         target_arch=None, default_timeout=default_timeout,
-        environment_variables=my_env_vars)
+        environment_variables=self._env_vars)
 
     self._perf_output_basedir = perf_output_basedir
 
@@ -184,6 +186,30 @@
         ]
     BuildFactory.__init__(self, build_factory_properties=properties)
 
+  def Validate(self):
+    """ Validate the Factory against the known good configuration. """
+    test_dir = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir,
+                            'tools', 'tests', 'factory_configuration')
+    expected_dir = os.path.join(test_dir, 'expected')
+    if not os.path.exists(expected_dir):

[rmistry, 2013/04/25 17:31:50]

I think we can assume that the expected dir always exists.

[borenet, 2013/04/25 18:28:09]

Done.

+      os.makedirs(expected_dir)
+    actual_dir = os.path.join(test_dir, 'actual')
+    if not os.path.exists(actual_dir):
+      os.makedirs(actual_dir)
+    expectation = None
+    try:
+      expectation = open(os.path.join(expected_dir, self._builder_name)).read()
+    except IOError:
+      # Should we error out here?  We really want to have expectations...

[borenet, 2013/04/25 15:50:28]

I'm in favor of throwing an error.  That way, we are forced to add/modify
expectation files every time we add/modify a builder.

[rmistry, 2013/04/25 17:31:50]

Yes lets throw an error, else we will never be able to see the below warning
with all the logs that are outputted.

+      print 'Warning: No expected factory configuration for %s.' % \
+          self._builder_name
+    as_string = utils.ToString(self.__dict__)

[rmistry, 2013/04/25 17:31:50]

as_string -> vars_as_string  or attr_as_string ?

[borenet, 2013/04/25 18:28:09]

Changed to self_as_string, since it really is a representation of the entire
object as a string.

+    with open(os.path.join(actual_dir, self._builder_name), 'w') as f:
+      f.write(as_string)
+    if expectation and as_string != expectation:

[rmistry, 2013/04/25 17:31:50]

if we throw an error above then expecation should always exist and we can do:
if as_string != expectation:

+      raise ValueError('Factory configuration for %s does not match '
+                       'expectation!' % self._builder_name)
+
   def AddSlaveScript(self, script, description, args=None, timeout=None,
                      halt_on_failure=False, is_upload_step=False,
                      is_rebaseline_step=False, get_props_from_stdout=None,
@@ -497,6 +523,7 @@
     self.CommonSteps(clobber)
     self.NonPerfSteps()
     self.PerfSteps()
+    self.Validate()

[borenet, 2013/04/25 15:50:28]

Each builder only uses one of these Build functions, so this is safe.  Calling
Validate() here rather than at the end of the constructor causes all of the
build steps to be included in the dump.

     return self
 
   def BuildCompileOnly(self, clobber=None):
@@ -509,6 +536,7 @@
     """
     self.UpdateSteps()
     self.Compile(clobber=clobber, build_in_one_step=False)
+    self.Validate()
     return self
 
   def BuildNoPerf(self, clobber=None):
@@ -519,6 +547,7 @@
     """
     self.CommonSteps(clobber)
     self.NonPerfSteps()
+    self.Validate()
     return self
 
   def BuildPerfOnly(self, clobber=None):
@@ -535,4 +564,5 @@
                        CONFIG_RELEASE)
     self.CommonSteps(clobber)
     self.PerfSteps()
+    self.Validate()
     return self