Create extra test steps for Perl modules

[nhemingway]

Buildbot's test steps currently only cover Perl modules in the general "make test" sense. Create PerlModuleTest? which understands testing perl modules.

[nhemingway]

The attached files resolves this.

The changes also implement changes to:

• add properties to steps rather than just builds - used to store the results of...
• interpret the output of the perl test and stores the number of tests passed and failed and the total number of tests in step properties.

[nhemingway]

darcs patch to resolve the ticket

[dustin]

It's not closed until it's in Brian's repo! :)

Do you mind posting the patch as a unified diff? I'm not darcs-savvy enough to use this format (darcs apply <patch didn't work). I will probably split the perl tests from the step properties and commit in two patches.

[nhemingway]

Oops, sorry. Here you go.

I also had some issues (I suspect you have more experience of darcs than I), until I realised that darcs had 'darcs diff -u'

I've split the two apart, and added a little documentation for step properties.

Given that marking the ticket closed is *not* the way to do it, how should I notify you/brian etc. that the patch is ready to go? Just email to the list?

[nhemingway]

darcs diff -u to add step properties

[nhemingway]

darcs diff -u to add PerlModuleTest?

[dustin]

updated patch against devel repository; doesn't pass tests

[dustin]

One more update and we should be ready to merge this!

[dustin]

OK, I fixed up the tests ({g,s}etStepProperty -> {g,s}etProperty) and committed to both the devel tree and brian's tree.

[nhemingway]

Dustin,

did you apply both step_props.diff and pmt.diff for this? Your repos seem to be missing the changes that were in step_props.diff. I've just patched my repos with it and (apart from one hunk that it didn't know where to put, the tests pass and it works

[dustin]

Sorry, I assumed, without really looking, that those were part of the properties workin #87 and #124, but 'tis not so. I'll take a look at the patch.

Is there a particular reason you don't want to store those properties in the build, where all other properties end up? I think that properties are confusing enough already :)

[nhemingway]

Several steps may want to have a property named the same. We could prefix step properties with the name of the step, but it just struck me as being cleaner for build properties to live with the build and step properties to live with steps.

That being said, all I'm using them for is statistics about steps and build (specifically, the number of tests that passed/failed etc). Maybe that use should be broken away from "properties" to "statistics". I didn't see a pressing need for it, so didn't do that

[dustin]

Well, if they're specific to the step, would any other part of the build process access them? Would it work to store the results simply as object attributes? Then !getText can simply access the attributes. You'll note, in the version I wrote, that I added a !getText method.

[nhemingway]

ISTR patching (whatever replaced) Waterfall to show the number of tests failed.

One of the other inspirations for it was that at work, (at the time) we had a codebase where lots of tests didn't pass, but we wanted to have the build run warn rather than fail if the number of failing tests was no worse than last time. However, this case has since gone away (it didn't take long to get the tests all passing)

[bhearsum]

Changeset 648 is breaking the Waterfall on trunk - can we get this backed out?

[dustin]

Yeah, that's my bad. Given that it's just doc breakage, I'd rather just fix it. I'll get to that today.

[bhearsum]

Are you sure it's just docs? I'm getting tracebacks related to sumStepProperty()

[dustin]

This is the only broken builder I see in the metabuildbot:  http://buildbot.buildbot.net/builders/docs/builds/194

Maybe I misinterpreted "breaking the Waterfall on trunk" to mean that the metabuildbot was showing failures. Can you paste the tracebacks? I can roll it back if it looks like it will take a while to sort out the problems, but if it's an easy fix I'd rather keep the momentum pointing forward :)

[bhearsum]

Here's the traceback I've been getting: 2008/06/25 09:13 PDT [HTTPChannel,6,10.2.72.11] 10.2.72.11 - - [25/Jun/2008:16:13:04 +0000] "GET /waterfall HTTP/1.1" 500 21862 "-" "Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9) Gecko/200806 1004 Firefox/3.0" 2008/06/25 09:13 PDT [HTTPChannel,6,10.2.72.11] Traceback (most recent call last):

File "/tools/twisted-2.4.0/lib/python2.5/site-packages/twisted/protocols/basic.py", line 232, in dataReceived

why = self.lineReceived(line)

File "/tools/twisted-2.4.0/lib/python2.5/site-packages/twisted/web/http.py", line 1004, in lineReceived

self.allContentReceived()

File "/tools/twisted-2.4.0/lib/python2.5/site-packages/twisted/web/http.py", line 1045, in allContentReceived

req.requestReceived(command, path, version)

File "/tools/twisted-2.4.0/lib/python2.5/site-packages/twisted/web/http.py", line 601, in requestReceived

self.process()

--- <exception caught here> ---

File "/tools/twisted-2.4.0/lib/python2.5/site-packages/twisted/web/server.py", line 160, in process

self.render(resrc)

File "/tools/twisted-2.4.0/lib/python2.5/site-packages/twisted/web/server.py", line 167, in render

body = resrc.render(self)

File "/tools/bens-buildbotlib/python2.5/site-packages/buildbot/status/web/base.py", line 246, in render

data = self.content(request)

File "/tools/bens-buildbotlib/python2.5/site-packages/buildbot/status/web/base.py", line 290, in content

data += self.body(request)

File "/tools/bens-buildbotlib/python2.5/site-packages/buildbot/status/web/waterfall.py", line 501, in body

box = ITopBox(b).getBox(request)

File "/tools/bens-buildbotlib/python2.5/site-packages/buildbot/status/web/waterfall.py", line 113, in getBox

tests_failed = b.sumStepProperty('tests-failed')

<type 'exceptions.AttributeError?'>: BuildStatus? instance has no attribute 'sumStepProperty'

[nhemingway]

This is caused by step_props.diff not having been applied

[dustin]

OK.. I really don't like the idea of adding another meaning for "properties" to the codebase. I also don't like the non-pythonic getPropertyOrDefault(..); the Properties class's getProperty() already supports a default, while the getitem function throws an exception when the property doesn't exist.

Will you kill me if I switch things up so that steps have "statistics" or "results", and a build has a method or two to summarize these statistics?

I notice that you're on gtalk. If you want to discuss this via IM, please feel free (I'm djmitche).

[dustin]

step stats + tests

[dustin]

Neil, here's tonight's work. It still needs

• documentation; and
• a number of steps should probably be modified to dump their data into statistics; and
• the web display of a step should show its stats

which I will get to shortly. This is in my repo as #228:step-statistics.patch.

[dustin]

Huh, so the problem is, in pmt.diff, the waterfall diff has code specific to the PerlModuleTest. This is OK for a class that's in the codebase, but not so helpful for user-supplied BuildStep? subclasses.

I wonder if we could key statistics such that combinable stats could be easily identified and combined, and then automatically summarize all statistics in the web display?

[nhemingway]

Although I haven't changed any other testing ShellCommands?, I thought that the concepts of tests-{failed,passed,total} were generic enough to be applicable to any test step, not just perl. From that perspective, is it not valid, if the step did indeed fail, to include the tests-failed stat in the Waterfall?

If you'd like, I can go and work out how to collect the statistics for the other languages currently supported by bb ShellCommands?? It would be good for me to learn the idiomatic way of testing in python, but it'll take a while as python is something I do at home, so progress can sometimes be slow.

I'm not saying that a more generic way of summarizng statistics wouldn't be good, just that the change for Waterfall isn't IMHO PerlModuleTest? specific.

[dustin]

That's a fair point. I'll document that those properties are specially represented in the waterfall.

[nhemingway]

Dustin,

The latest state of this seems to be that the Waterfall doesn't display the number of failing tests? I ask as buildbot-commit tells me it's been pushed to Brian's tree. Are you planning to come back to it post 0.7.8?

[dustin]

"I'll be baack"

Yeah, there's a good bit more work to do, I just haven't gotten around to it.

[dustin]

OK, neil, do you want to take a look at brian's tree in about an hour, or my tree now? I still need to do documentation, but that doesn't block release.

[Pike]

tests_failed = b.getSummaryStatistic('failed', operator.add)

needs to be

tests_failed = b.getSummaryStatistic('failed', operator.add, 0)

or something like that, I just died on this locally.

[dustin]

Thanks, good point. I was thinking that not passing an initial value would work, but I was being dumb. :)

[warner]

Fixed, in [619c521dea1a396292ee016dcde3658349b81472].