add a better fix for describe called before rendered bug

We just describe a little bit later instead of accepting
not rendered arguments

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>

master/buildbot/data/steps.py

@@ -99,11 +99,6 @@ def startConsuming(self, callback, options, kwargs):
             raise NotImplementedError("cannot consume from this path")
 
 
-class UrlType(types.Entity):
-    name = types.String()
-    url = types.String()
-
-
 class Step(base.ResourceType):
 
     name = "step"

master/buildbot/process/buildstep.py

@@ -275,6 +275,7 @@ class BuildStep(results.ResultComputingConfigMixin,
     progress = None
     logEncoding = None
     cmd = None
+    rendered = False  # true if attributes are rendered
 
     def __init__(self, **kwargs):
         for p in self.__class__.parms:
@@ -310,6 +311,7 @@ def _describe(self, done=False):
         return [self.name]
 
     def describe(self, done=False):
+        assert(self.rendered)
         desc = self._describe(done)
         if self.descriptionSuffix:
             desc = desc + self.descriptionSuffix
@@ -351,8 +353,6 @@ def setProgress(self, metric, value):
 
     @defer.inlineCallbacks
     def setStateStrings(self, strings):
-        # we render the strings with properties first
-        strings = yield self.build.render(strings)
         yield self.master.data.updates.setStepStateStrings(self.stepid, map(unicode, strings))
         # call to the status API for now
         self.step_status.old_setText(strings)
@@ -385,10 +385,6 @@ def startStep(self, remote):
                         " parent Build (%s)" % (l, self, self.build))
                 raise RuntimeError("lock claimed by both Step and Build")
 
-        # Set the step's text here so that the stepStarted notification sees
-        # the correct description
-        # self.description is not yet rendered, but setStateStrings accepts renderable
-        yield self.setStateStrings(self.describe(False))
         self.step_status.stepStarted()
 
         try:
@@ -421,6 +417,9 @@ def setRenderable(res, attr):
                 d.addCallback(setRenderable, renderable)
                 dl.append(d)
             yield defer.gatherResults(dl)
+            self.rendered = True
+            # we describe ourselves only when renderables are interpolated
+            yield self.setStateStrings(self.describe(False))
 
             # run -- or skip -- the step
             if doStep:
@@ -457,7 +456,10 @@ def setRenderable(res, attr):
             # At the same time we must respect RETRY status because it's used
             # to retry interrupted build due to some other issues for example
             # due to slave lost
-            descr = self.describe(True)
+            if self.rendered:
+                descr = self.describe(True)
+            else:  # we haven't rendered yet. Don't bother
+                descr = []
             if results == CANCELLED:
                 yield self.setStateStrings(descr + ["cancelled"])
             else:

master/buildbot/test/unit/test_process_buildstep.py

@@ -254,10 +254,12 @@ def test_describe(self):
         descriptionDone = ['oogaBooga done!']
         step = buildstep.BuildStep(description=description,
                                    descriptionDone=descriptionDone)
+        step.rendered = True
         self.assertEqual(step.describe(), description)
         self.assertEqual(step.describe(done=True), descriptionDone)
 
         step2 = buildstep.BuildStep()
+        step2.rendered = True
         self.assertEqual(step2.describe(), [step2.name])
         self.assertEqual(step2.describe(done=True), [step2.name])
 
@@ -269,11 +271,13 @@ def test_describe_suffix(self):
         step = buildstep.BuildStep(description=description,
                                    descriptionDone=descriptionDone,
                                    descriptionSuffix=descriptionSuffix)
+        step.rendered = True
         self.assertEqual(step.describe(), description + descriptionSuffix)
         self.assertEqual(step.describe(done=True),
                          descriptionDone + descriptionSuffix)
 
         step2 = buildstep.BuildStep(descriptionSuffix=descriptionSuffix)
+        step2.rendered = True
         self.assertEqual(step2.describe(), [step2.name] + descriptionSuffix)
         self.assertEqual(step2.describe(done=True),
                          [step2.name] + descriptionSuffix)
@@ -814,6 +818,5 @@ def test_description(self):
             ExpectShell(workdir='build', command=['foo', 'BAR'])
             + 0,
         )
-        # TODO: status is only set at the step start, so BAR isn't rendered
-        self.expectOutcome(result=SUCCESS, status_text=["'foo'"])
+        self.expectOutcome(result=SUCCESS, status_text=["'foo", "BAR'"])
         yield self.runStep()

master/buildbot/test/unit/test_steps_mswin.py

@@ -39,6 +39,7 @@ def tearDown(self):
     def _run_simple_test(self, source, destination, expected_args=None, expected_code=0, expected_res=SUCCESS, **kwargs):
         s = mswin.Robocopy(source, destination, **kwargs)
         self.setupStep(s)
+        s.rendered = True
         self.assertEqual(s.describe(), ['???'])
         self.assertEqual(s.describe(done=True), ['???'])
 

master/buildbot/test/unit/test_steps_shell.py

@@ -80,108 +80,128 @@ def test_constructor_args_validity(self):
 
     def test_describe_no_command(self):
         step = shell.ShellCommand(workdir='build')
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['???'],) * 2)
 
     def test_describe_from_empty_command(self):
         # this is more of a regression test for a potential failure, really
         step = shell.ShellCommand(workdir='build', command=' ')
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['???'],) * 2)
 
     def test_describe_from_short_command(self):
         step = shell.ShellCommand(workdir='build', command="true")
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'true'"],) * 2)
 
     def test_describe_from_short_command_list(self):
         step = shell.ShellCommand(workdir='build', command=["true"])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'true'"],) * 2)
 
     def test_describe_from_med_command(self):
         step = shell.ShellCommand(command="echo hello")
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'echo", "hello'"],) * 2)
 
     def test_describe_from_med_command_list(self):
         step = shell.ShellCommand(command=["echo", "hello"])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'echo", "hello'"],) * 2)
 
     def test_describe_from_long_command(self):
         step = shell.ShellCommand(command="this is a long command")
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'this", "is", "...'"],) * 2)
 
     def test_describe_from_long_command_list(self):
         step = shell.ShellCommand(command="this is a long command".split())
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'this", "is", "...'"],) * 2)
 
     def test_describe_from_nested_command_list(self):
         step = shell.ShellCommand(command=["this", ["is", "a"], "nested"])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'this", "is", "...'"],) * 2)
 
     def test_describe_from_nested_command_tuples(self):
         step = shell.ShellCommand(command=["this", ("is", "a"), "nested"])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'this", "is", "...'"],) * 2)
 
     def test_describe_from_nested_command_list_empty(self):
         step = shell.ShellCommand(command=["this", [], ["is", "a"], "nested"])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'this", "is", "...'"],) * 2)
 
     def test_describe_from_nested_command_list_deep(self):
         step = shell.ShellCommand(command=[["this", [[["is", ["a"]]]]]])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'this", "is", "...'"],) * 2)
 
     def test_describe_custom(self):
         step = shell.ShellCommand(command="echo hello",
                                   description=["echoing"], descriptionDone=["echoed"])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['echoing'], ['echoed']))
 
     def test_describe_with_suffix(self):
         step = shell.ShellCommand(command="echo hello", descriptionSuffix="suffix")
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'echo", "hello'", 'suffix'],) * 2)
 
     def test_describe_custom_with_suffix(self):
         step = shell.ShellCommand(command="echo hello",
                                   description=["echoing"], descriptionDone=["echoed"],
                                   descriptionSuffix="suffix")
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['echoing', 'suffix'], ['echoed', 'suffix']))
 
     def test_describe_no_command_with_suffix(self):
         step = shell.ShellCommand(workdir='build', descriptionSuffix="suffix")
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['???', 'suffix'],) * 2)
 
     def test_describe_unrendered_WithProperties(self):
         step = shell.ShellCommand(command=properties.WithProperties(''))
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['???'],) * 2)
 
     def test_describe_unrendered_custom_new_style_class_rendarable(self):
         step = shell.ShellCommand(command=object())
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['???'],) * 2)
 
     def test_describe_unrendered_custom_old_style_class_rendarable(self):
         class C:
             pass
         step = shell.ShellCommand(command=C())
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (['???'],) * 2)
 
     def test_describe_unrendered_WithProperties_list(self):
         step = shell.ShellCommand(
             command=['x', properties.WithProperties(''), 'y'])
+        step.rendered = True
         self.assertEqual((step.describe(), step.describe(done=True)),
                          (["'x", "y'"],) * 2)
 
@@ -954,10 +974,12 @@ def test_setTestResults(self):
 
     def test_describe_not_done(self):
         step = self.setupStep(shell.Test())
+        step.rendered = True
         self.assertEqual(step.describe(), ['testing'])
 
     def test_describe_done(self):
         step = self.setupStep(shell.Test())
+        step.rendered = True
         self.step_statistics['tests-total'] = 93
         self.step_statistics['tests-failed'] = 10
         self.step_statistics['tests-passed'] = 20
@@ -967,6 +989,7 @@ def test_describe_done(self):
 
     def test_describe_done_no_total(self):
         step = self.setupStep(shell.Test())
+        step.rendered = True
         self.step_statistics['tests-total'] = 0
         self.step_statistics['tests-failed'] = 10
         self.step_statistics['tests-passed'] = 20

master/docs/developer/cls-buildsteps.rst

@@ -94,6 +94,12 @@ BuildStep
         The log encoding to use for logs produced in this step, or None to ues the global default.
         See :ref:`Log-Encodings`.
 
+    .. py:attribute:: rendered
+
+        At the begining of the step, the renderable attributes are rendered against the properties.
+        There is a slight delay however when those are not yet rendered, which lead to weird and difficult to reproduce bugs. To address this problem, a ``rendered`` attribute is
+        available for methods that could be called early in the buildstep creation.
+
     A few important pieces of information are not available when a step is constructed, and are added later.
     These are set by the following methods; the order in which these methods are called is not defined.
 
@@ -398,7 +404,7 @@ BuildStep
         This allows a step to provide links to data that is not available in the log files.
 
     Build steps have a set of short strings associated with them, describing the current status and results.
-    
+
     .. py:method:: setStateStrings(strings)
 
         :param strings: a list of short strings

master/docs/manual/cfg-properties.rst

@@ -119,7 +119,7 @@ Using Properties in Steps
 
 For the most part, properties are used to alter the behavior of build steps
 during a build.  This is done by annotating the step definition in
-``master.cfg`` with placeholders.  When the step is executed, these
+``master.cfg`` with placeholders.  When the step is started, these
 placeholders will be replaced using the current values of the build properties.
 
 .. note::
@@ -352,13 +352,13 @@ syntaxes in the parentheses.
 ``propname:~replacement``
     Like ``propname:-replacement``, but only substitutes the value
     of property ``propname`` if it is something Python regards as ``True``.
-    Python considers ``None``, 0, empty lists, and the empty string to be 
+    Python considers ``None``, 0, empty lists, and the empty string to be
     false, so such values will be replaced by ``replacement``.
 
 ``propname:+replacement``
     If ``propname`` exists, substitute ``replacement``; otherwise,
     substitute an empty string.
-    
+
 Although these are similar to shell substitutions, no other
 substitutions are currently supported, and ``replacement`` in the
 above cannot contain more substitutions.