[zorg] r174135 - LitTestCommand: If the test command fails, always report failure.

[David Blaikie]

On Fri, Feb 1, 2013 at 9:10 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> On Fri, Feb 1, 2013 at 8:13 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Thu, Jan 31, 2013 at 5:39 PM, Daniel Dunbar <daniel at zuster.org> wrote:
>> > Author: ddunbar
>> > Date: Thu Jan 31 19:39:42 2013
>> > New Revision: 174135
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=174135&view=rev
>> > Log:
>> > LitTestCommand: If the test command fails, always report failure.
>>
>> Have you seen this in the wild? Where/why?
>
>
> Yes. This is just a good fallback, otherwise its possible for a failure to
> go unnoticed.
>
>>
>> Shouldn't we do something a
>> bit more severe if this happens - seems to represent a fairly
>> confusing situation for any user.
>
>
> Reporting failure in the buildbot is the appropriate thing to do, in my
> opinion.

I'm of varying opinion on this - reporting something to buildbot is
good, but we didn't log anything extra to explain that this is sort of
catastrophic. Something is misconfigured here if we didn't see any log
failures but we did see a process failure. From a "we're running lit
to get results" perspective, this is a test configuration/buildbot
configuration failure, not a failure in one of the user's tests.

I'd be inclined to report some kind of more specific failure -
possibly buildbot.status.results.EXCEPTION (same thing that would be
reported if the test assert fails, I think). Same reason I'd just
'assert' when we see a context for a different test from the last log
line.

While this conflates buildbot infrastructure bugs with build
infrastructure bugs (as you say, might've been the wrong command
inside the build files) I think that's a better separation (& keeps
the code simpler - asserting rather than 'handling' these strange
cases) than conflating test failures with build infrastructure
failures under normal 'failures'.

Though I'm not strongly wedded to this - at the very least it'd be
useful to log an extra something somewhere the same way we do for the
context/result mismatch.

- David

> This situation can easily come up if someone put in the wrong lit command,
> or anything else where lit or the tests would fail to run but not
> necessarily produce any test failures.
>
>  - Daniel
>
>>
>> >
>> > Modified:
>> >     zorg/trunk/zorg/buildbot/commands/LitTestCommand.py
>> >
>> > Modified: zorg/trunk/zorg/buildbot/commands/LitTestCommand.py
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/zorg/trunk/zorg/buildbot/commands/LitTestCommand.py?rev=174135&r1=174134&r2=174135&view=diff
>> >
>> > ==============================================================================
>> > --- zorg/trunk/zorg/buildbot/commands/LitTestCommand.py (original)
>> > +++ zorg/trunk/zorg/buildbot/commands/LitTestCommand.py Thu Jan 31
>> > 19:39:42 2013
>> > @@ -76,8 +76,14 @@ class LitTestCommand(Test):
>> >      self.addLogObserver('stdio', self.logObserver)
>> >
>> >    def evaluateCommand(self, cmd):
>> > +    # Always report failure if the command itself failed.
>> > +    if cmd.rc != 0:
>> > +      return FAILURE
>> > +
>> > +    # Otherwise, report failure if there were failures in the log.
>> >      if self.logObserver.failed:
>> > -        return FAILURE
>> > +      return FAILURE
>> > +
>> >      return SUCCESS
>> >
>> >    def describe(self, done=False):
>> > @@ -97,6 +103,10 @@ class StepProxy(object):
>> >    def addCompleteLog(self, name, text):
>> >      self.logs.append((name, text))
>> >
>> > +class RemoteCommandProxy(object):
>> > +  def __init__(self, rc):
>> > +    self.rc = rc
>> > +
>> >  class TestLogObserver(unittest.TestCase):
>> >    def parse_log(self, text):
>> >      observer = LitLogObserver()
>> > @@ -115,5 +125,18 @@ PASS: test-three (3 of 3)
>> >      self.assertEqual(obs.resultCounts, { 'FAIL' : 1, 'PASS' : 2 })
>> >      self.assertEqual(obs.step.logs, [('test-two', 'FAIL: test-two (2 of
>> > 3)')])
>> >
>> > +class TestCommand(unittest.TestCase):
>> > +  def parse_log(self, text):
>> > +    cmd = LitTestCommand()
>> > +    cmd.logObserver.step = StepProxy()
>> > +    for ln in text.split('\n'):
>> > +      cmd.logObserver.outLineReceived(ln)
>> > +    return cmd
>> > +
>> > +  def test_command_status(self):
>> > +    # If the command failed, the status should always be error.
>> > +    cmd = self.parse_log("")
>> > +    self.assertEqual(cmd.evaluateCommand(RemoteCommandProxy(1)),
>> > FAILURE)
>> > +
>> >  if __name__ == '__main__':
>> >    unittest.main()
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>