Merge lp://qastaging/~dooferlad/offspring/passing-ssh-key-to-slave2 into lp://qastaging/offspring

Some minor fixes needed and it's good to go...

#1

+ def test_currentBuildIsActive(self):

This is actually four tests hidden in one...can you split them out, your comments are really the docstrings for the other tests...

#2
+ def test_addUserToBzrSshUrl(self):

This is cool, but I'd prefer to see the comparison made more explicit, you know what the result should be, so it might be better to be explicit in the test...

expected_url = "bzr+ssh://<email address hidden>/~dooferlad/offspring/config"

Using the same technique as the implementation is no guarantee that the result is actually correct :-)

#3

+ def test_currentBuildIsActive(self):

Doesn't fit with the test naming conventions (pep8)

#4

There are a couple of minor conflicts with trunk, can merge trunk and resolve?

Other than these minor issues, looks good, nice branch :-)

On 9 December 2011 07:42, Kevin McDermott <email address hidden> wrote:

> #3
>
> +    def test_currentBuildIsActive(self):
>
> Doesn't fit with the test naming conventions (pep8)

Please clarify. It fits with the naming convention of all the other
tests doesn't it?

Thanks,

--
James Tunnicliffe

Hi James,

    def test_currentBuildIsActive(self):

compared with...

    def test_slave_run_starts_server_and_logs(self):
    def test_slave_stop(self):
    def test_slave_stop_build_no_current_build(self):
    def test_slave_stop_build_with_current_build(self):
    def test_slave_start_build(self):
    def test_currentBuildIsActive(self):
    def test_addUserToBzrSshUrl(self):
    def test_start_build_with_custom_ssh_key(self):
    def test_check_ssh_key_deleted_when_build_finished(self):
    def test__check_ssh_agent(self):

Also (and I think this was my fault), test__check_ssh_agent, can you rename that to test_check_ssh_agent ?

thanks,

Kevin :-)

I was mostly thinking about test_addUserToBzrSshUrl(self): the other
example :-) Would you suggest test_add_user-_to_bzr_ssh_url then?

I have already fixed up the __ problem.

James

On 9 December 2011 12:03, Kevin McDermott <email address hidden> wrote:
> Hi James,
>
>    def test_currentBuildIsActive(self):
>
> compared with...
>
>    def test_slave_run_starts_server_and_logs(self):
>    def test_slave_stop(self):
>    def test_slave_stop_build_no_current_build(self):
>    def test_slave_stop_build_with_current_build(self):
>    def test_slave_start_build(self):
>    def test_currentBuildIsActive(self):
>    def test_addUserToBzrSshUrl(self):
>    def test_start_build_with_custom_ssh_key(self):
>    def test_check_ssh_key_deleted_when_build_finished(self):
>    def test__check_ssh_agent(self):
>
> Also (and I think this was my fault), test__check_ssh_agent, can you rename that to test_check_ssh_agent ?
>
> thanks,
>
> Kevin :-)
> --
> https://code.launchpad.net/~dooferlad/offspring/passing-ssh-key-to-slave2/+merge/84987
> You are the owner of lp:~dooferlad/offspring/passing-ssh-key-to-slave2.

--
James Tunnicliffe