Renderable locks master #1670

vlovich · 2015-05-17T09:09:36Z

As requested, this change cherry-picked onto master.

jaredgrubb · 2015-05-17T15:20:00Z

I was going to request tests, but I dont see any unit tests in "test_process_buildstep" dealing with locks right now, so this may not be easy. I'll leave it up to you.

vlovich · 2015-05-17T16:51:58Z

I did look for tests to use as a template and didn't find any. If you have any suggestions on how to do that I'd be interested

jaredgrubb · 2015-05-17T18:47:34Z

svn test "test_mode_incremental_repourl_renderable" is a test for renderable things.
shell test "test_describe_unrendered_WithProperties_list" also.

I dunno... up to you if you want to try to make it all work. You could have it render a Mock type and just verify that some method gets called.

Anyway, usually we have a strict "add unit tests" requirement for contributions, but this is a gray one and maybe not easy ... Our code would certainly be better off for it if you want to :) Would make it easier to do more tests in the future. Up to you.

sa2ajj · 2015-05-17T19:50:06Z

@vlovich this feature did not appear out of nowhere, did it? :) I'd suggest looking at the way it's used and base the test on that.

jaredgrubb · 2015-05-17T19:54:02Z

@sa2ajj I think it's just there's not a lot of testing around it and he'd have to write some of that basis himself .. which is a good thing in the end, but it is a little involved.

sa2ajj · 2015-05-17T20:04:34Z

I agree it might require a bit of effort, I'd like to hear the estimate based on some practical approach though.

@vlovich please spend a bit of time to see how much effort it's gonna take.

vlovich · 2015-05-17T20:29:00Z

One problem I've had is that running the tests locally there are failures. How do I know if they work or not?

PYTHONPATH=. trial buildbot.test

RealConfigs
    test_0_7_12_config ...                                                 [OK]
    test_0_7_6_config ...                                                  [OK]
    test_sample_config ...                                              [ERROR]
                                             [ERROR]
buildbot.test.integration.test_custom_buildstep
  RunSteps
    test_Latin1ProducingCustomBuildStep ...                                [OK]
    test_OldBuildEPYDoc ...                                                [OK]
    test_OldPerlModuleTest ...                                             [OK]
    test_OldStyleCustomBuildStep ...                                       [OK]
    test_OldStyleCustomBuildStepSlowDB ...                                 [OK]
    test_OldStyleCustomBuildStep_failure ...                               [OK]
    test_step_raising_buildstepfailed_in_start ...                         [OK]
    test_step_raising_connectionlost_in_start ...                          [OK]
    test_step_raising_exception_in_start ...                               [OK]
buildbot.test
  integration
    test_customservices ...                                             [ERROR]
    test_integration_template ...                                       [ERROR]
    test_mailnotifier ...                                               [ERROR]
    test_master ...                                                     [ERROR]
    test_setproperyfromcommand ...                                      [ERROR]
buildbot.test.integration.test_slave_comm
  TestSlaveComm
    test_connect_disconnect ...                                            [OK]
    test_duplicate_slave ...                                          [SKIPPED]
    test_duplicate_slave_old_dead ...                                      [OK]
buildbot.test
  integration
    test_stop_trigger ...                                               [ERROR]
    test_transfer ...                                                   [ERROR]
    test_trigger ...                                                    [ERROR]
    test_try_client ...                                                 [ERROR]
buildbot.test.integration.test_upgrade
  TestPickles
    test_sourcestamp_081 ...                                               [OK]
    test_sourcestamp_version3 ...                                          [OK]
  TestWeirdChanges
    testUpgradeChangeNoRevision ...                                        [OK]
    testUpgradeChangeProperties ...                                        [OK]
    testUpgradeListsAsFilenames ... ^C                                    [ERROR]

jaredgrubb · 2015-05-17T20:31:13Z

I've seen failures in "integration" as well. Some tips:

remove pyc files if you're swaping between 8 & 9
if you still see errors, run on the main branch (eight/master) and note what the failures are ... then re-run on your branch and verify the failures are the same. Then you know it's not you :)

vlovich · 2015-05-17T20:36:47Z

I do still see the same errors on master. It still doesn't leave me with a lot of confidence with making changes if tests are randomly failing only locally. Automation does not appear to have this problem...

vlovich · 2015-05-17T21:26:08Z

@sa2ajj I agree there needs to be unit tests. I don't think my original suggestion would even work. It looks like the attributes are rendered after locks are acquired which is too late.

I've tried writing a test but I feel like I'm really off on a tangent at this point & there must be an easier way.

Here's what I have so far:

     @defer.inlineCallbacks                                               
     def test_renderableLocks(self):                                      
         from buildbot import locks                                       
         from buildbot.process.properties import renderer                 

         lock1 = mock.Mock(spec=locks.MasterLock)                         
         lock1.name = "masterlock"                                        

         lock2 = mock.Mock(spec=locks.SlaveLock)                          
         lock2.name = "slavelock"                                         

         renderedLocks = [False]                                          

         @renderer                                                        
         def rendered_locks(props):                                       
             renderedLocks[0] = True                                      
             access1 = locks.LockAccess(lock1, 'counting')                
             access2 = locks.LockAccess(lock2, 'exclusive')               
             return [access1, access2]                                    

         self.setupStep(self.FakeBuildStep(locks=rendered_locks))         
         yield self.runStep()                                             

         self.assertTrue(renderedLocks[0])

It fails with:

Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/twisted/internet/defer.py", line 1099, in _inlineCallbacks
    result = g.send(result)
  File "/Users/vlovich/projects/buildbot/master/buildbot/process/buildstep.py", line 450, in startStep
    for l, la in self.locks]
exceptions.AttributeError: FakeBuild instance has no attribute 'slavebuilder'

buildbot.test.unit.test_process_buildstep.TestBuildStep.test_renderableLocks
-------------------------------------------------------------------------------

vlovich · 2015-05-17T21:51:17Z

OK. Took my best shot at writing a unit test. A couple of fakes were insufficient but hopefully I did it right. Turns out the implementation was wrong as the locks are acquired before we render the properties.

tardyp · 2015-05-18T06:23:15Z

master/buildbot/process/buildstep.py

@@ -438,6 +438,8 @@ def startStep(self, remote):
            name=util.ascii2unicode(self.name))
        yield self.master.data.updates.startStep(self.stepid)

+        self.locks = yield self.build.render(self.locks)


Can you please add a test where the locks are not renderable? I'm not sure this line will work in that case.

tardyp · 2015-05-18T06:25:13Z

test looks good. Need another one to make me confident.

sa2ajj · 2015-05-18T16:42:35Z

I think it looks good. I agree w/ @tardyp about a non-renderable locks + I'd appreciate a phrase (or two) in the release notes.

sa2ajj · 2015-05-20T12:46:20Z

test for non-renderable lock

(I'll move this todo to somebody who does it :))

Setup the slavebuilder & locks as well.

Setup the botmaster attribute.

The expected usage is that someone would create a renderable that returns a list of locks so that this list can be dynamic.

vlovich · 2015-05-21T19:30:51Z

Seems like tests pass even with non-renderable lock. Was there some reason to think that x.render(y) doesn't work when y is not a deferred value? It would seem like it would have to given how renderables in buildstep.py work.

tardyp · 2015-05-21T20:10:21Z

👍

Renderable locks master

tardyp · 2015-06-10T20:54:35Z

Hi looks like some users see that this change break their conf.
http://trac.buildbot.net/ticket/3277

vlovich · 2015-06-10T20:57:17Z

The logs indicate buildbot-0.8.13 but only 0.8.12 has been released. Did someone try to backport this to 0.8.12? There's a separate pull request for that but it's blocked on some kind of test failure I can't figure out.

vlovich mentioned this pull request May 17, 2015

Renderable locks #1669

Merged

sa2ajj added the needs work label May 17, 2015

vlovich force-pushed the renderable-locks-master branch from 1b6ce89 to 3deae56 Compare May 17, 2015 21:50

tardyp reviewed May 18, 2015
View reviewed changes

sa2ajj added the needs more review label May 18, 2015

Vitali Lovich added 2 commits May 21, 2015 10:41

Make the FakeBuild a more convincing fake

9940e63

Setup the slavebuilder & locks as well.

Make the FakeMaster a more convincing fake

c8a4214

Setup the botmaster attribute.

vlovich force-pushed the renderable-locks-master branch from 3deae56 to 4a910f6 Compare May 21, 2015 17:41

Vitali Lovich added 3 commits May 21, 2015 12:12

Add unit test to test renderable locks.

952a354

Make sure that locks are renderable.

83bf42c

The expected usage is that someone would create a renderable that returns a list of locks so that this list can be dynamic.

Update documentation

9393a94

vlovich force-pushed the renderable-locks-master branch from 4a910f6 to 9393a94 Compare May 21, 2015 19:29

sa2ajj pushed a commit that referenced this pull request May 22, 2015

Merge pull request #1670 from vlovich/renderable-locks-master

874ed59

Renderable locks master

sa2ajj merged commit 874ed59 into buildbot:master May 22, 2015

sa2ajj added merge-me and removed needs work labels May 22, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renderable locks master #1670

Renderable locks master #1670

vlovich commented May 17, 2015

jaredgrubb commented May 17, 2015

vlovich commented May 17, 2015

jaredgrubb commented May 17, 2015

sa2ajj commented May 17, 2015

jaredgrubb commented May 17, 2015

sa2ajj commented May 17, 2015

vlovich commented May 17, 2015

jaredgrubb commented May 17, 2015

vlovich commented May 17, 2015

vlovich commented May 17, 2015

vlovich commented May 17, 2015

tardyp May 18, 2015

tardyp commented May 18, 2015

sa2ajj commented May 18, 2015

sa2ajj commented May 20, 2015

vlovich commented May 21, 2015

tardyp commented May 21, 2015

tardyp commented Jun 10, 2015

vlovich commented Jun 10, 2015

Renderable locks master #1670

Renderable locks master #1670

Conversation

vlovich commented May 17, 2015

jaredgrubb commented May 17, 2015

vlovich commented May 17, 2015

jaredgrubb commented May 17, 2015

sa2ajj commented May 17, 2015

jaredgrubb commented May 17, 2015

sa2ajj commented May 17, 2015

vlovich commented May 17, 2015

jaredgrubb commented May 17, 2015

vlovich commented May 17, 2015

vlovich commented May 17, 2015

vlovich commented May 17, 2015

tardyp May 18, 2015

Choose a reason for hiding this comment

tardyp commented May 18, 2015

sa2ajj commented May 18, 2015

sa2ajj commented May 20, 2015

vlovich commented May 21, 2015

tardyp commented May 21, 2015

tardyp commented Jun 10, 2015

vlovich commented Jun 10, 2015