1133685 - Increase mozharness test coverage

Status: RESOLVED INCOMPLETE

(Release Engineering :: Applications: MozharnessCore, defect, P3)

Description

[Massimo Gervasini [:massimo]]

I have just run:

tox; coverage report -m

from a clean mozharness checkout, it says the test coverage is 14%.

We should increase the test coverage at least for core modules: mozharness/base and mozharness/mozilla

Comment 1

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1133685 - Added tests for TransferMixin.patch

Added tests from TransferMixin

Comment 2

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8569095 [details] [diff] [review]
[mozharness] Bug 1133685 - Added tests for TransferMixin.patch

Review of attachment 8569095 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Massimo,

Thanks for taking the time to write this!

tests++

The tests are great - I think you've covered every scenario!

The only thing that we're not really testing is whether the generated rsync command is valid, I wonder if we could do that too. For example, we could pass fixed strings through:

tm.rsync_upload_directory(
local_path='my/local/path',
ssh_key='../../mysshkey_rsa',
ssh_user='colinthetomato',
remote_host='tomatoalerting.nsa.org',
remote_path='/watch/out/they/will/splat',)

and then intercept the generated rsync command and check it against exactly what you think it should be...

The existing tests do a great job of handling the combinations of successful/unsuccessful uploads/downloads for local dirs/non-dirs and I think with one or two tests around the generation of the rsync command, we should have really good coverage here.

Thanks Massimo!
Pete

::: mozharness/base/transfer.py
@@ +78,4 @@
>                                   error_level=ERROR,
>                                   ):
>          """
> +        rsync+ssh the content of a remote directory to local_path

The tests check specific return values (-1, -2, -3, None) and therefore would be good if the docstring also explained what each of them means.

::: test/test_base_transfer.py
@@ +59,5 @@
> +                         remote_path='remote path',
> +                         create_remote_directory=True), -2)
> +
> +    @mock.patch('mozharness.base.transfer.os')
> +    def test_rsync_upload_faild_do_not_create_remote_dir(self, os_mock):

s/faild/fails/

Comment 3

[Pete Moore [:pmoore][:pete]]

P.S. and about the exit code explanations in the docstring - that applies to both the upload and download method.  Thanks!

Comment 4

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1133685 - Added tests for TransferMixin II.patch

Thanks for the review, Pete.

Here's the updated version of this patch.

I did not include tests for the rsync command; I agree we should test the rsync command, but to do it properly, we need to refactor the TransferMixin class, adding a _create_rsync_command() method. 
This exactly the goal of tests, finding weak points of our code and improve it. I'll file a separate bug about adding the new method to the TransferMixin

Thanks again!

Comment 5

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8570470 [details] [diff] [review]
[mozharness] Bug 1133685 - Added tests for TransferMixin II.patch

Awesome!!

Thanks Massimo!

Comment 6

[Aki Sasaki (not active)]

No activity for 4 years, and mozharness looks like it's headed for eventual deprecation. Let's resolve incomplete.