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!'

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

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

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?

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.

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().

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

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?

(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 '/').

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

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)".

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

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 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 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.

Keeping r+

Keeping r+.

added test on regular dir as well.

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 "/".

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).

(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.

Should be fine now.

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.

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.

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

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

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.