868505 - [mozdevice] DeviceManagerSUT.fileExists(path) throws unhelpful exception if path is directory or does not exist

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Mark Côté [:mcote]]

If you call DeviceManagerSUT.fileExists() and pass in a path to a directory or to a path that does not exist, you get a big long exception that is ultimately not very helpful.

It should instead return False.  Also, I would like the documentation for DeviceManager.fileExists() to be updated to say "and is a regular file", since directories are technically files on POSIX systems. (e.g. stat() differentiates between regular files, directories, pipes, etc.).

Finally, I would like both a unit test and a sut_test for this.

Traceback (most recent call last):
  File "/Users/mcote/projects/mozbase/bin/dm", line 8, in <module>
    load_entry_point('mozdevice==0.22', 'console_scripts', 'dm')()
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/dmcli.py", line 334, in cli
    cli.run(args)
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/dmcli.py", line 138, in run
    ret = args.func(args)
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/dmcli.py", line 278, in listfiles
    if self.dm.fileExists(args.remote_dir) and not self.dm.dirExists(args.remote_dir):
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/devicemanagerSUT.py", line 402, in fileExists
    return s[-1] in self.listFiles(containingpath)
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/devicemanagerSUT.py", line 406, in listFiles
    if (self.dirExists(rootdir) == False):
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/devicemanagerSUT.py", line 391, in dirExists
    ret = self._runCmds([{ 'cmd': 'isdir ' + remotePath }]).strip()
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/devicemanagerSUT.py", line 152, in _runCmds
    self._sendCmds(cmdlist, outputfile, timeout, retryLimit=retryLimit)
  File "/Users/mcote/projects/mozbase/src/mozbase/mozdevice/mozdevice/devicemanagerSUT.py", line 134, in _sendCmds
    raise err
mozdevice.devicemanager.DMError: Automation Error: Error processing command 'isdir '; err='Wrong number of arguments for isdir command!'

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix fileExists and listFiles

Used :mcote's code from bug 797672 for the listfiles part.

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: sut test

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: unit test

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: sut test

Improve the sut test by creating a file that gets pushed to the device.

Comment 5

[Mark Côté [:mcote]]

Comment on attachment 745288 [details] [diff] [review]
fix fileExists and listFiles

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

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +398,4 @@
>          # Because we always have / style paths we make this a lot easier with some
>          # assumptions
>          s = filepath.split('/')
> +        containingpath = '/'.join(s[:-1]) or '/'

This works, but it's really kludgy, and this is a good opportunity to continue replacing sometimes-confusing path manipulation through join() and split() calls with use of the posixpath module.  posixpath.split() gives us the directory and the filename in one call, and it's completely clear what we're doing.  Do you mind making that change?

Comment 6

[Mark Côté [:mcote]]

Gah, and actually fileExists() does *not* check if the path is a regular file, only that it exists.  pathExists() probably would have been a better name... but too late now.  So remove the docstring change too please.  We can implement a different function if need be (e.g. regularFileExists()) although it would probably require SUT support.  A separate bug in any case.

Comment 7

[Mark Côté [:mcote]]

Another thought: we will also have to call posixpath.normpath() to remove any trailing slashes.  The current version doesn't handle trailing slashes properly either.  It will also have to handle '/', which it doesn't currently do (nor with the attached patch).  It should always exist, but to be paranoid we can still do the listFiles(); if that failed there would be something seriously wrong (permissions or worse).  But I think fileExists('/') is going to have to be a special case, since '/' or '' won't show up in the result of listFiles().

Comment 8

[Mark Côté [:mcote]]

Sorry, one more comment--the doc should be updated to clearly indicate that it returns True if the path exists, regardless of file type.

Wow was this function both really broken and easily misunderstood! :D

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix dm docstring

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use posixpath in dmsut

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

I'm not sure how to handle "/" properly (I mean in a way that will actually show permission errors and what not) with what SUT offers now. Maybe we should open a different bug addressing this?

Comment 12

[Mark Côté [:mcote]]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #11)
> I'm not sure how to handle "/" properly (I mean in a way that will actually
> show permission errors and what not) with what SUT offers now. Maybe we
> should open a different bug addressing this?

As I mentioned, one way would be to do the listFiles() on '/' but return True regardless of what listFiles() returns.  If there is an error reading the directory, it should generate a DMError, which is good enough.  Another way would be to ensure that dirExists('/') returns True.  Basically we just want to make sure we can read the root directory.

You can file a separate bug if you want, but we're already fixing a couple different things here, so I'm fine with it being included in this bug.  Actually, a good argument for fixing it here is that we can commit full, proper unit tests (don't forget to update them for directories, including '/').

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use posixpath in dmsut; make fileExists work on /

direxists already works on / from what I can see btw.

Comment 14

[Mark Côté [:mcote]]

Comment on attachment 748219 [details] [diff] [review]
use posixpath in dmsut; make fileExists work on /

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

Good one with small change.

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +398,5 @@
>          # Because we always have / style paths we make this a lot easier with some
>          # assumptions
> +        filepath = posixpath.normpath(filepath)
> +        if filepath == '/':
> +            self.listFiles(filepath)

I actually only suggested listFiles() because this function already calls it, and I thought you might work the fix around that.  If you're going to go with this flow (which is fine), we might as well just call self.dirExists(filepath) here, which takes less work (presumably) than listing the contents of the directory (which we don't actually care about here).  Also add a comment explaining why we have the special case, i.e., because even though '/' should always exist, we want to make sure we can access it, but it's not in the output of listFiles('/').
 
So just "return self.dirExists(filepath)".

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use posixpath in dmsut; make fileExists work on /

Wasn't sure if I could carry forward the r+ or not. :)

Comment 16

[Mark Côté [:mcote]]

Comment on attachment 748385 [details] [diff] [review]
use posixpath in dmsut; make fileExists work on /

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

Yeah, I think the general rule is that an r+ means that no re-review is necessary, as long as there are only minor changes (if requested). :) Anyway looks good.

Comment 17

[Mark Côté [:mcote]]

Comment on attachment 745309 [details] [diff] [review]
unit test

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

I'm r+ing this as long as the assertion is changed (see below).  Also, please add the test to mozdevice/tests/manifest.ini as well.

::: mozdevice/tests/sut_fileExists.py
@@ +10,5 @@
> +
> +    def test_onRoot(self):
> +        a = MockAgent(self, commands=self.commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=a.port)
> +        self.assertFalse(d.fileExists('/'))

This should now be assertTrue().

Comment 18

[Mark Côté [:mcote]]

Comment on attachment 745326 [details] [diff] [review]
sut test

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

Again this doesn't work as it currently is, but it only needs one change to fix that.  Also, please include a test on a regular directory.  r+ with those changes.

::: mozdevice/sut_tests/test_fileExists.py
@@ +11,5 @@
> +    """This tests the "fileExists" command.
> +    """
> +
> +    def testOnRoot(self):
> +        self.assertFalse(self.dm.fileExists('/'))

Should be assertTrue() now.

Comment 19

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: unit test

Keeping r+

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: sut test

Keeping r+.

Comment 21

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: sut test

added test on regular dir as well.

Comment 22

[Mark Côté [:mcote]]

Comment on attachment 748573 [details] [diff] [review]
unit test

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

Heh when I asked you to change that assertFalse to assertTrue, I assumed you would also run the test to make sure nothing else was required...

::: mozdevice/tests/sut_fileExists.py
@@ +10,5 @@
> +
> +    def test_onRoot(self):
> +        a = MockAgent(self, commands=self.commands)
> +        d = mozdevice.DroidSUT("127.0.0.1", port=a.port)
> +        self.assertTrue(d.fileExists('/'))

This fails because fileExists() no longer does the "cd" if the path is "/".

Comment 23

[Mark Côté [:mcote]]

Two things:

* If you get an r+ with minor changes required, generally you don't need to post the new patch; just check it in and post a link to the changeset.  You only need to post a new patch if you found that you had to make bigger changes that originally anticipated, in which case you would go back to an r? for a full review.

* Never post patches without testing them, no matter how minor the change!  Just because a reviewer said that only a minor change is required, there may be other changes that were missed (as in this case--I meant that it fileExists("/") should now return True, but there was another change required to get that to work in that test).

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Mark Côté ( :mcote ) from comment #22)
> Comment on attachment 748573 [details] [diff] [review]
> unit test
> 
> Review of attachment 748573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heh when I asked you to change that assertFalse to assertTrue, I assumed you
> would also run the test to make sure nothing else was required...
> 
> ::: mozdevice/tests/sut_fileExists.py
> @@ +10,5 @@
> > +
> > +    def test_onRoot(self):
> > +        a = MockAgent(self, commands=self.commands)
> > +        d = mozdevice.DroidSUT("127.0.0.1", port=a.port)
> > +        self.assertTrue(d.fileExists('/'))
> 
> This fails because fileExists() no longer does the "cd" if the path is "/".

Ha! I actually did but the spaghetti of patches I had applied messed my results. Will fix.

Comment 25

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: unit test

Should be fine now.

Comment 26

[Mark Côté [:mcote]]

Comment on attachment 748879 [details] [diff] [review]
unit test

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

Cool thanks.  Don't forget to rerun the test if you make the change I suggest below. :)

::: mozdevice/tests/sut_fileExists.py
@@ +3,5 @@
> +import unittest
> +
> +class FileExistsTest(unittest.TestCase):
> +
> +    commands = [('isdir /', 'TRUE'),

Only comment is that maybe this should be called "nonroot_commands" or something like that, just to make the difference between these commands and the ones in test_onRoot clear.

Comment 27

[Mark Côté [:mcote]]

Btw I was thinking that I should do a try run with this fix and the fix from bug 868574 before you check anything in.  They *shouldn't* break anything, but I'd like to be positive.

Comment 28

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: unit test

Made the change, it's root_commands now. Tests pass.

Comment 29

[Mihnea Dobrescu-Balaur (:mihneadb)]

:mcote, I think you can do that try run now.

Comment 30

[Mark Côté [:mcote]]

Looks like it ran fine, just one failure, which is a known intermittent.  Mihnea, go ahead and commit all this to the mozbase github repo.

Comment 31

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://github.com/mozilla/mozbase/commit/321bc0422c1d728850fb8a855650bb023ac18cb0