Ticket #228 (closed enhancement: fixed)

Opened 5 months ago

Last modified 2 months ago

Create extra test steps for Perl modules

Reported by: nhemingway Assigned to: nhemingway
Priority: blocker Milestone: 0.7.8
Component: other Version: 0.7.6
Keywords: Cc: dustin

Description

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

Attachments

perl_module_test.darcs_diff (6.4 kB) - added by nhemingway on 04/04/08 13:15:42.
darcs patch to resolve the ticket
step_props.diff (6.4 kB) - added by nhemingway on 04/06/08 01:27:19.
darcs diff -u to add step properties
pmt.diff (7.0 kB) - added by nhemingway on 04/06/08 01:35:40.
darcs diff -u to add PerlModuleTest?
228.patch (5.6 kB) - added by dustin on 05/16/08 14:13:17.
updated patch against devel repository; doesn't pass tests
228-step-statistics.patch (5.9 kB) - added by dustin on 06/25/08 20:32:08.
step stats + tests

Change History

04/04/08 13:08:33 changed by nhemingway

  • status changed from new to assigned.

04/04/08 13:14:48 changed by 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.

04/04/08 13:15:42 changed by nhemingway

  • attachment perl_module_test.darcs_diff added.

darcs patch to resolve the ticket

04/04/08 13:16:33 changed by nhemingway

  • status changed from assigned to closed.
  • resolution set to fixed.

04/04/08 15:37:40 changed by dustin

  • status changed from closed to reopened.
  • resolution deleted.

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.

04/06/08 01:25:35 changed by 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?

04/06/08 01:27:19 changed by nhemingway

  • attachment step_props.diff added.

darcs diff -u to add step properties

04/06/08 01:35:40 changed by nhemingway

  • attachment pmt.diff added.

darcs diff -u to add PerlModuleTest?

05/16/08 14:13:17 changed by dustin

  • attachment 228.patch added.

updated patch against devel repository; doesn't pass tests

05/16/08 14:13:50 changed by dustin

  • cc set to dustin.

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

05/26/08 17:07:26 changed by dustin

  • status changed from reopened to closed.
  • resolution set to fixed.

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

06/09/08 03:52:23 changed by 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

06/09/08 06:50:47 changed by 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 :)

06/09/08 07:20:20 changed by 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

06/09/08 07:35:10 changed by dustin

  • status changed from closed to reopened.
  • resolution deleted.

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.

06/09/08 08:23:53 changed by 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)

06/25/08 09:27:23 changed by bhearsum

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

06/25/08 09:46:59 changed by 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.

06/25/08 09:49:09 changed by bhearsum

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

06/25/08 09:56:16 changed by 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 :)

06/25/08 09:59:06 changed by 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-buildbot//lib/python2.5/site-packages/buildbot/status/web/base.py", line 246, in render

data = self.content(request)

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

data += self.body(request)

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

box = ITopBox(b).getBox(request)

File "/tools/bens-buildbot//lib/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'

06/25/08 10:40:46 changed by nhemingway

This is caused by step_props.diff not having been applied

06/25/08 11:44:46 changed by 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).

06/25/08 20:32:08 changed by dustin

  • attachment 228-step-statistics.patch added.

step stats + tests

06/25/08 20:33:45 changed by 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.

06/29/08 20:56:40 changed by 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?

06/30/08 11:56:57 changed by 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.

06/30/08 19:47:47 changed by dustin

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

07/15/08 12:27:29 changed by dustin

  • milestone changed from undecided to 0.7.8.

07/15/08 13:28:37 changed by 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?

07/15/08 13:51:29 changed by dustin

"I'll be baack"

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

07/21/08 10:47:21 changed by 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.

07/24/08 06:25:34 changed by Pike

  • priority changed from minor to blocker.

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.

07/24/08 08:09:13 changed by dustin

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

07/24/08 16:24:16 changed by warner

  • status changed from reopened to closed.
  • resolution set to fixed.

Fixed, in [669].