Fix AbstractLatentBuildslave

[benallard]

Those are fix that were needed to get the Dockerstuff working.

See TRAC-2970

[sa2ajj]

It looks like some bits were missed during a refactoring, this points to the lack of tests.

@djmitche, @jaredgrubb, everybody else, any suggestions where to start to create proper tests?

[sa2ajj]

I kinda wonder how it worked... Probably the affected functionality is not common in the normal setup (and tests passed).

[djmitche]

This patch looks good. A lot of this was from the incomplete work on the new master-slave protocol -- work that was actually reverted on master with the expectation that it would be fixed up and re-merged before nine. I'm sad that never happened. That project also added some more testable interfaces along with better tests.

I think that the best way to catch this sort of thing is integration testing -- hook up a master and a fake slave and make something happen. Much of that is already built in buildbot.test.integration.test_slave_comm, but that only tests some of the connection-handling code (specifically around duplicate slaves).

[sa2ajj]

@djmitche are there any copies of that work?

[djmitche]

Yes -- I think https://github.com/buildbot/buildbot/tree/slave-proto is most up to date, and maybe also https://github.com/tomprince/buildbot/tree/slaveproto. @tomprince is a good person to talk to, although as you can see from the commit dates, it's been well over a year.

[djmitche]

I'm setting merge-me because I think, even without tests, this is a good fix.

[sa2ajj]

I'll leave @benallard to decide whether to merge it as it is or if he wants to try to add a test case.

[benallard]

Of course, I want to add a test-case ! I'd love to see coverage improve in this area ! However I'm not sure how complicated it is to add, nor when I'll find time to seat down and look at it. Best I guess is to merge this, and add an issue in TRAC about the need for an integration test for latent slaves ...

[sa2ajj]

Added ticket:3016.