Add ability to copy files/trees to devicemanager

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ahal, Assigned: kyr0)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=ahal])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
There are a bunch of places in b2g automation where we need to run:
self.dm._checkShellOutput(['dd', 'if=%s' % in_file, 'of=%s' % out_file])

It would be nice if devicemanager offered an abstraction for this. I don't think there is any reason to have separate copyfile/copytree methods, one 'copy' method would be simpler.

Might be good to add a move method as well, but not really necessary.
(Reporter)

Comment 1

5 years ago
This is a bug in devicemanager: https://github.com/mozilla/mozbase/tree/master/mozdevice

Basically in order to fix it, we need to create a nice wrapper function around "self.dm._checkShellOutput(['dd', 'if=%s' % in_file, 'of=%s' % out_file])", similar to how other mozdevice functions do it (see https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerADB.py)

Devicemanager is an interface with two supported implementations (ADB and SUT). So copyfile functionality would need to be implemented in both.

Feel free to ping me with more questions if needed, ahal on irc.
Whiteboard: [good first bug][mentor=ahal]
(Reporter)

Comment 2

4 years ago
Sebastiaan has offered to help out with this.
Assignee: nobody → sebastiaan
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 833135 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-files-trees.patch

This patch is a work in progress concept. Please critique.
Attachment #833135 - Flags: review?(ahalberstadt)
(Assignee)

Comment 4

4 years ago
Created attachment 833145 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-fol.patch

This time with the right patch. Still WIP.
Attachment #833135 - Attachment is obsolete: true
Attachment #833135 - Flags: review?(ahalberstadt)
Attachment #833145 - Flags: review?(ahalberstadt)
(Reporter)

Comment 5

4 years ago
Comment on attachment 833145 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-fol.patch

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

The approach looks good! There are a few things that need fixing up, but this seems to be on the right track. Let me know if you need help testing this out.

::: mozdevice/mozdevice/devicemanager.py
@@ +284,5 @@
>  
>      @abstractmethod
> +    def moveTree(self, source, destination):
> +         """
> +         Does a move of the directory on the device

Please add documentation for the parameters using the sphinx syntax (e.g :param foo: description). Same for all the other methods you are adding

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +271,5 @@
> +        self._runCmd(["shell", "mv", "-f", source, destination])
> +        if self._dirExists(source):
> +            self._removeDir(source)
> +        else:
> +            self._removeFile(source)

These don't have an underscore prefix

@@ +278,5 @@
> +        try:
> +            self._checkShellOutput(['dd', 'if=%s' % source,
> +                    'of=%s' % destination])
> +        except:
> +            raise "Something went wrong during the copy process."

Don't put a blanket except that hides the actual error. Either let it propagate, or catch a specific error that we know how to handle and handle it.

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +425,5 @@
>          if self.dirExists(remoteDir):
>              self._runCmds([{ 'cmd': 'rmdr ' + remoteDir }])
>  
> +    def moveTree(self, source, destination):
> +        self._runCmd(["shell", "mv", "-f", source, destination])

This isn't going to work for SUT agent (I don't think). SUT has it's own special syntax. If you need to access the shell you can use self.shellCheckOutput().

@@ +433,5 @@
> +            self._removeFile(source)
> +
> +    def copyTree(self, source, destination):
> +        try:
> +            self._checkShellOutput(['dd', 'if=%s' % source,

No underscore

@@ +436,5 @@
> +        try:
> +            self._checkShellOutput(['dd', 'if=%s' % source,
> +                    'of=%s' % destination])
> +        except:
> +            raise "Something went wrong during the copy process."

Again, just let the error propagate unless there is a good reason to handle it.
Attachment #833145 - Flags: review?(ahalberstadt) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8334472 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-folders

I think I nailed it. Both the ADB tests and SUT tests come back ok.
Attachment #833145 - Attachment is obsolete: true
Attachment #8334472 - Flags: review?(ahalberstadt)
(Assignee)

Comment 7

4 years ago
I Don't know what you would prefer in the devicemanager.py method description. That is the one nit I have myself :)
(Assignee)

Comment 8

4 years ago
Created attachment 8334485 [details] [diff] [review]
Removed a debug line in DeviceManagerADB
Attachment #8334472 - Attachment is obsolete: true
Attachment #8334472 - Flags: review?(ahalberstadt)
Attachment #8334485 - Flags: review?
(Reporter)

Comment 9

4 years ago
Comment on attachment 8334485 [details] [diff] [review]
Removed a debug line in DeviceManagerADB

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

This is awesome! The only thing you need to change is to use _checkCmd instead of _runCmd in dmADB. Please upload one last patch with this and the whitespace fixes.

Though for bonus points, you could add some assertions to your tests to check to make sure the files are actually getting copied (and that the original is removed in the case of moveTree). Hint: you can use d.fileExists(). For even more bonus points you could add a test for copying/moving non-empty directories. This part is optional though if you don't want.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +267,5 @@
>          else:
>              self.removeFile(remoteDir.strip())
>  
> +    def moveTree(self, source, destination):
> +        self._runCmd(["shell", "mv", source, destination])

I think we need to use _checkCmd here. The difference is _runCmd won't wait for the operation to finish before continuing whereas _checkCmd will. Since cp or mv could potentially take awhile, I think we want to wait.

@@ +270,5 @@
> +    def moveTree(self, source, destination):
> +        self._runCmd(["shell", "mv", source, destination])
> +
> +    def copyTree(self, source, destination):
> +        self._runCmd(["shell", "dd", "if=%s" % source, "of=%s" % destination])

Same as above

::: mozdevice/tests/sut_copytree.py
@@ +9,5 @@
> +
> +        m = MockAgent(self, commands=commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=m.port, logLevel=mozlog.DEBUG)
> +        # No error imples we're all good
> +        self.assertEqual(None, d.copyTree('/mnt/sdcard/tests/test.txt', 

nit: extra whitespace

@@ +18,5 @@
> +
> +        m = MockAgent(self, commands=commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=m.port, logLevel=mozlog.DEBUG)
> +        # No error imples we're all good
> +        self.assertEqual(None, d.copyTree('/mnt/sdcard/tests', 

nit: extra whitespace

::: mozdevice/tests/sut_movetree.py
@@ +9,5 @@
> +
> +        m = MockAgent(self, commands=commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=m.port, logLevel=mozlog.DEBUG)
> +        # No error imples we're all good
> +        self.assertEqual(None, d.moveTree('/mnt/sdcard/tests/test.txt', 

nit: whitespace

@@ +18,5 @@
> +
> +        m = MockAgent(self, commands=commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=m.port, logLevel=mozlog.DEBUG)
> +        # No error imples we're all good
> +        self.assertEqual(None, d.moveTree('/mnt/sdcard/tests', 

nit: whitespace
Attachment #8334485 - Flags: review?
(Reporter)

Comment 10

4 years ago
Comment on attachment 8334485 [details] [diff] [review]
Removed a debug line in DeviceManagerADB

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

I think the method descriptions are fine. Noticed a typo though:

::: mozdevice/mozdevice/devicemanager.py
@@ +286,5 @@
> +    def moveTree(self, source, destination):
> +         """
> +         Does a move of the file or directory on the device.
> +
> +        :param source: Path to the orinal file or directory

typo: original

@@ +295,5 @@
> +    def copyTree(self, source, destination):
> +         """
> +         Does a copy of the file or directory on the device.
> +
> +        :param source: Path to the orinal file or directory

same as above
(Assignee)

Comment 11

4 years ago
Created attachment 8335183 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-fol.patch

Nits adressed. Hopefully bonuspoints attained. Please check the tests carefully I am not sure if I did it correctly.


I was trying to use the dirExists when checking if a directory existed but it kept returning null, fileExists however worked perfectly.
Attachment #8334485 - Attachment is obsolete: true
Attachment #8335183 - Flags: review?(ahalberstadt)
(Assignee)

Comment 12

4 years ago
Created attachment 8335227 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-folders

Nit: Made the test directories between the methods the same.
Attachment #8335183 - Attachment is obsolete: true
Attachment #8335183 - Flags: review?(ahalberstadt)
Attachment #8335227 - Flags: review?(ahalberstadt)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8335227 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-folders

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

Thank you! There are a few changes to make to the tests though. I wasn't actually very familiar with sut tests either, and it turns out they are rather complicated :). But think of it this way, the commands list at the top is emulating an actual filesystem. So pretend that all those commands are actually getting executed and you want to make the responses actually match what you'd expect the filesystem to return. So when running the 'ls' command, the response should be a list of every file in the current directory you'd expect to be there based on the commands that were previously run.

::: mozdevice/tests/sut_copytree.py
@@ +14,5 @@
> +    def test_copyFile(self):
> +        commands = [('dd if=/mnt/sdcard/tests/test.txt of=/mnt/sdcard/tests/test2.txt', ''),
> +                    ('isdir /mnt/sdcard/tests', 'TRUE'),
> +                    ('cd /mnt/sdcard/tests', ''),
> +                    ('ls', 'test2.txt')]

This response should be test.txt and test2.txt, separated by newlines like: https://github.com/mozilla/mozbase/blob/master/mozdevice/tests/sut_list.py#L11

@@ +27,5 @@
> +    def test_copyDir(self):
> +        commands = [('dd if=/mnt/sdcard/tests/foo of=/mnt/sdcard/tests/bar', ''),
> +                    ('isdir /mnt/sdcard/tests', 'TRUE'),
> +                    ('cd /mnt/sdcard/tests', ''),
> +                    ('ls', 'bar')]

Same, foo and bar since it is a copy.

@@ +42,5 @@
> +        commands = [('isdir /mnt/sdcard/tests/foo/bar', 'TRUE'),
> +                    ('dd if=/mnt/sdcard/tests/foo of=/mnt/sdcard/tests/foo2', ''),
> +                    ('isdir /mnt/sdcard/tests', 'TRUE'),
> +                    ('cd /mnt/sdcard/tests', ''),
> +                    ('ls', 'foo2'),

foo and foo2

::: mozdevice/tests/sut_movetree.py
@@ +14,5 @@
> +    def test_moveFile(self):
> +        commands = [("mv /mnt/sdcard/tests/test.txt /mnt/sdcard/tests/test1.txt", ""),
> +                    ('isdir /mnt/sdcard/tests', 'TRUE'),
> +                    ('cd /mnt/sdcard/tests', ''),
> +                    ('ls', '/mnt/sdcard/tests/test.txt')]

I think the response here should just be 'test1.txt'

@@ +20,5 @@
> +        m = MockAgent(self, commands=commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=m.port, logLevel=mozlog.DEBUG)
> +        self.assertEqual(None, d.moveTree('/mnt/sdcard/tests/test.txt',
> +                '/mnt/sdcard/tests/test1.txt'))
> +        self.assertFalse(d.fileExists('/mnt/sdcard/tests/test.txt'))

Maybe also add an assertTrue that 'test1.txt' does exist

@@ +26,5 @@
> +    def test_moveDir(self):
> +        commands = [("mv /mnt/sdcard/tests/foo /mnt/sdcard/tests/bar", ""),
> +                    ('isdir /mnt/sdcard/tests', 'TRUE'),
> +                    ('cd /mnt/sdcard/tests', ''),
> +                    ('ls', '/mnt/sdcard/tests')]

I think this response should just be bar

@@ +39,5 @@
> +        commands = [('isdir /mnt/sdcard/tests/foo/bar', 'TRUE'),
> +                    ('mv /mnt/sdcard/tests/foo /mnt/sdcard/tests/foo2', ''),
> +                    ('isdir /mnt/sdcard/tests', 'TRUE'),
> +                    ('cd /mnt/sdcard/tests', ''),
> +                    ('ls', '/mnt/sdcard/tests/foo'),

This should just be foo2
Attachment #8335227 - Flags: review?(ahalberstadt) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 8335322 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-folders

Tests adjusted according to the comments.
Attachment #8335227 - Attachment is obsolete: true
Attachment #8335322 - Flags: review?(ahalberstadt)
(Reporter)

Comment 15

4 years ago
Comment on attachment 8335322 [details] [diff] [review]
0001-Bug-867654-Add-ability-to-copy-or-move-files-and-folders

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

Looks good and the tests pass locally for me!
Attachment #8335322 - Flags: review?(ahalberstadt) → review+
(Reporter)

Comment 16

4 years ago
Landed: https://github.com/mozilla/mozbase/commit/57a1c6dd00cc945e3274b0b17aba93fb3000cc43

Thanks once again!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.