While investigating a related issue with b2gpopulate, I couldn't understand why we made the changes in bug 1000918 and then pretty much ignored them in bug 999506. From my testing, there's no harm in having the 'tests' folder included in the device root, but if we insist on removing it I think it would be better to strip it, rather than to hard-code the locations in two places. One risk I can see is that additional files may exist parallel to 'tests' but our tests wouldn't push those, and cleanup_gaia will still cleanup any media files from other locations, so we should be safe. I also noticed that test_cleanup_sdcard.py is a false positive because it's actually relying on cleanup_gaia due to the files pushed being media files, so I'll fix that at the same time.

(In reply to Dave Hunt (:davehunt) from comment #0) > I also noticed that test_cleanup_sdcard.py is a false positive because it's > actually relying on cleanup_gaia due to the files pushed being media files, > so I'll fix that at the same time. Actually, this isn't entirely true, but it is specifically checking that only media files are removed, so I will enhance it to check for any files in the device root.

Would appreciate someone testing this with a Flame. I have triggered an adhoc job here: http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/2/ (currently in the queue)

Thanks Bob, I'm a little concerned by the adhoc results. I've triggered a new one without tests that require a carrier as I understand we still have issues with the RIL on Flame: http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/3/ It's currently in the queue. In the meantime, if you could run some more gallery tests that would give me some more confidence in this patch for Flame. Thanks!

As requested, I ran all of the tests in the /gallery folder and they all passed: TEST-START test_gallery_crop_photo.py test_gallery_crop_photo (test_gallery_crop_photo.TestGalleryCropPhoto) ... ok ---------------------------------------------------------------------- Ran 1 test in 39.482s OK TEST-START test_gallery_delete_image.py test_gallery_delete_image (test_gallery_delete_image.TestGalleryDelete) ... ok ---------------------------------------------------------------------- Ran 1 test in 37.200s OK TEST-START test_gallery_edit_photo.py test_gallery_edit_photo (test_gallery_edit_photo.TestGalleryEditPhoto) ... ok ---------------------------------------------------------------------- Ran 1 test in 40.261s OK TEST-START test_gallery_empty.py test_empty_gallery (test_gallery_empty.TestGalleryEmpty) https://moztrap.mozilla.org/manage/case/4003/ ... ok ---------------------------------------------------------------------- Ran 1 test in 33.016s OK TEST-START test_gallery_flick.py test_gallery_full_screen_image_flicks (test_gallery_flick.TestGallery) https://moztrap.mozilla.org/manage/case/1326/ ... ok ---------------------------------------------------------------------- Ran 1 test in 44.934s OK TEST-START test_gallery_view.py test_gallery_view (test_gallery_view.TestGallery) https://moztrap.mozilla.org/manage/case/2476/ ... ok ---------------------------------------------------------------------- Ran 1 test in 38.344s OK SUMMARY ------- passed: 6 failed: 0 todo: 0

Thanks Bob. The latest ad-hoc results confirm there's an issue with the gallery tests. Looking at the HTML report (attached), it would appear that there are several photos and a video that haven't been cleaned up. I've logged into the box with the device attached and can't see these files in either internal or external storage. Could you perhaps try running the camera tests followed by the gallery tests on the Flame to see if you can replicate? If you can replicate, please also try without my patch too.

I can confirm the same behaviour on the Flame. When I run with your branch the photos/videos from the Camera tests remain on the sdcard which cause the Gallery tests to fail. When I switch back to the master branch the problem goes away - the photos and videos are cleared. I did the same sequence on the Hamachi and everything works on both branches, so this is a problem specific to the Flame. Let me know if you want any help debugging this as I have a Flame (obviously).

Here's a clue: The photos on the flame are being stored on Internal Storage as opposed to SD Card Storage (as can be seen in Settings -> Media Storage). That might explain why cleaning up the sd card does not remove them. Although, the photos are stored on Internal Storage for each branch (master and yours), so it does seem like it's an issue with the cleanup routine working on master and not with your branch.

The way I understand it, everything is going onto internal storage by default with the Flame. We're probably rather erroneously calling it 'sdcard' in the tests as we previously didn't have internal storage available. Bob: Could you run a camera test and then list the contents of /storage/sdcard0 and /storage/sdcard1? I suspect my new patch is removing anything in the tests subdirectory but leaving any files in the top level directory. This would also imply that the removal of media files is not working for camera captures. Perhaps you could also add a print statement in cleanup_gaia to see what media files we're trying (and possibly silently failing) to delete.

Here is the result of `ls -lR` in `storage` after running a test that takes a photo: root@msm8610:/storage # ls -lR .: dr-xrwx--- system sdcard_rw 2014-05-28 16:47 sdcard0 dr-xrwx--- system media_rw 1970-01-01 01:00 sdcard1 ./sdcard0: dr-xrwx--- system sdcard_rw 2014-05-28 16:46 DCIM dr-xrwx--- system sdcard_rw 2014-05-28 16:47 tests ./sdcard0/DCIM: dr-xrwx--- system sdcard_rw 2014-05-28 16:46 100MZLLA ./sdcard0/DCIM/100MZLLA: -r-xrwx--- system sdcard_rw 465164 2014-05-28 16:46 IMG_0001.jpg ./sdcard0/tests: dr-xrwx--- system sdcard_rw 2014-05-28 16:47 DCIM ./sdcard0/tests/DCIM: dr-xrwx--- system sdcard_rw 2014-05-28 16:47 100MZLLA ./sdcard0/tests/DCIM/100MZLLA: -r-xrwx--- system sdcard_rw 355856 2013-11-01 19:52 IMG_0001.jpg ./sdcard1: dr-xrwx--- system media_rw 1980-01-01 00:00 LOST.DIR dr-xrwx--- system media_rw 2014-05-07 16:43 tests ./sdcard1/LOST.DIR: ./sdcard1/tests: I also added a statement into cleanup_gaia to print `self.data_layer.media_files`, then each filename as it was being attempted to be deleted, and then `self.data_layer.media_files` again. The output was: files to remove: [u'/sdcard/DCIM/100MZLLA/IMG_0001.jpg'] attempting to remove /sdcard/DCIM/100MZLLA/IMG_0001.jpg files remaining: [u'/sdcard/DCIM/100MZLLA/IMG_0001.jpg'] So it appears as if the delete is failing silently. Let me know if there is anything else I can do to help.

Hmm, I think the above was a bit confusing as the test I ran after the camera test was crop_photo and I think it added its own file. I believe that's why there are two jpgs in different locations above. I did another tests where I ran a test that doesn't push any files and here is the result: root@msm8610:/storage # ls -lR .: dr-xrwx--- system sdcard_rw 2014-05-28 16:54 sdcard0 dr-xrwx--- system media_rw 1970-01-01 01:00 sdcard1 ./sdcard0: dr-xrwx--- system sdcard_rw 2014-05-28 16:54 DCIM dr-xrwx--- system sdcard_rw 2014-05-28 16:53 tests ./sdcard0/DCIM: dr-xrwx--- system sdcard_rw 2014-05-28 16:54 100MZLLA ./sdcard0/DCIM/100MZLLA: -r-xrwx--- system sdcard_rw 410673 2014-05-28 16:54 IMG_0001.jpg ./sdcard0/tests: ./sdcard1: dr-xrwx--- system media_rw 1980-01-01 00:00 LOST.DIR dr-xrwx--- system media_rw 2014-05-07 16:43 tests ./sdcard1/LOST.DIR: ./sdcard1/tests: And the console output: files to remove: [u'/sdcard/DCIM/100MZLLA/IMG_0001.jpg'] attempting to remove /sdcard/DCIM/100MZLLA/IMG_0001.jpg files remaining: [u'/sdcard/DCIM/100MZLLA/IMG_0001.jpg']

Thanks Bob! Speaking to :dhylands on IRC it seems our method to remove all media files is working only be chance. The device storage API hides the real location of volumes, but on some systems the obscured name happens to be a valid path to the correct file. In the case of Flame, where we have two storage locations, we get /sdcard/... which is not a real location. It would seem the best way to fix this is to use the device storage API to remove the files. We most probably did not hit this before because our device root really was the root and by deleting everything in it, we cleared all the media files anyway. There may be a good case here for restoring the device root exclude the /tests directory. I'll tackle both issues in my next patch, and will fix the test_cleanup_gaia unit test so it checks the media files are removed. I'm not sure why it doesn't already.

I've updated my patch so we're no longer relying on the removal of media files (see bug 1017521 for the reasons). We now use deviceRoot when pushing to the device, but use any known storage path when performing the cleanup. Once bug 1018079 is resolved we can then remove the hard-coded storage paths. I'll request review again once I have results from the following adhoc run against Flame: http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/4/

I made a silly mistake in my last patch. I pushed a fix, and retriggered an adhoc job: http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/5/ (currently in the queue)

Comment on attachment 8425488 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19442 The adhoc run hasn't finished but it's given me the confidence I needed for this patch. The Travis-CI failure was in a different test suite.

Landed in: https://github.com/mozilla-b2g/gaia/commit/54d6512f516564c23e4363ca328b2a453605617b