Closed
Bug 1133685
Opened 9 years ago
Closed 3 years ago
Increase mozharness test coverage
Categories
(Release Engineering :: Applications: MozharnessCore, defect, P3)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: massimo, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
6.22 KB,
patch
|
pmoore
:
review+
massimo
:
checked-in+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
Added tests from TransferMixin
Attachment #8569095 -
Flags: review?(pmoore)
Comment 2•9 years ago
|
||
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/
Attachment #8569095 -
Flags: review?(pmoore) → review+
Comment 3•9 years ago
|
||
P.S. and about the exit code explanations in the docstring - that applies to both the upload and download method. Thanks!
Reporter | ||
Comment 4•9 years ago
|
||
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!
Attachment #8570470 -
Flags: review?(pmoore)
Reporter | ||
Updated•9 years ago
|
Attachment #8569095 -
Attachment is obsolete: true
Comment 5•9 years ago
|
||
Comment on attachment 8570470 [details] [diff] [review] [mozharness] Bug 1133685 - Added tests for TransferMixin II.patch Awesome!! Thanks Massimo!
Attachment #8570470 -
Flags: review?(pmoore) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8570470 -
Flags: checked-in+
Updated•7 years ago
|
Priority: -- → P3
Comment 6•3 years ago
|
||
No activity for 4 years, and mozharness looks like it's headed for eventual deprecation. Let's resolve incomplete.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•