1017521 - Use DeviceStorage.delete to remove media files instead of ADB

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

At the moment we're obtaining a list of files using DeviceStorage.enumerate and then using ADB to remove these files. This works only if the file names returned by DeviceStorage.enumerate match the files in the device's file system. For Flame we have two storage locations (internal and external), and this has highlighted this issue. We need to fix our unit tests so we pick up on this issue, and switch to using DeviceStorage.delete to remove the files.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19764

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I need to look into the impact of running this against B2G desktop where media files are found from the file system. If this patch would delete those files then I think we should guard it so we don't attempt to remove files on B2G desktop. Once we have bug 984340 then we should be able to reintroduce this as it will be limited to the files in the fake sdcard path.

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8430740 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19764

I added a couple of questions to the PR, but they aren't blockers.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm glad I checked this - running desktop B2G without a storage override uses paths common you your operating system. This means that enumerate will return those files, and delete will actually delete them. Even once we're using storage override I don't feel it's worth the risk of deleting files so my next patch will guard this so it won't run on desktop B2G.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8430740 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19764

Requesting another review because I also have a specific request. Could you try running the test_cleanup_gaia unit test against Flame without the other changes? It might be easiest to just make the same changes locally to your master branch. I am expecting the test to fail.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Regarding the Travis-CI failures: I think this may be due to running the enumerate before desktop B2G was ready. It doesn't really matter because we're not executing this code on desktop B2G with my latest patch.

Comment 7

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #5)

> Requesting another review because I also have a specific request. Could you
> try running the test_cleanup_gaia unit test against Flame without the other
> changes? It might be easiest to just make the same changes locally to your
> master branch. I am expecting the test to fail.

As requested on Flame:

TEST-START test_cleanup_gaia.py
test_cleanup_gaia (test_cleanup_gaia.TestCleanupGaia) ... FAIL

======================================================================
FAIL: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/zac/.virtualenvs/gaiatest/local/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/marionette_test.py", line 163, in run
    testMethod()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cleanup_gaia.py", line 51, in test_cleanup_gaia
    self.check_initial_state()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cleanup_gaia.py", line 66, in check_initial_state
    self.assertEqual(self.data_layer.media_files, [])
AssertionError: Lists differ: [u'/sdcard/IMG_0001_1.jpg', u'... != []

First list contains 3 additional elements.
First extra element 0:
/sdcard/IMG_0001_1.jpg

+ []
- [u'/sdcard/IMG_0001_1.jpg',
-  u'/sdcard/IMG_0001_2.jpg',
TEST-UNEXPECTED-FAIL | test_cleanup_gaia.py test_cleanup_gaia.TestCleanupGaia.test_cleanup_gaia | -  u'/sdcard/IMG_0001_3.jpg']

Comment 8

[Bob Silverberg [:bsilverberg]]

The whole branch passes on my Flame. Testing just the changed test against the master branch yields the same failure as Zac:

TEST-START test_cleanup_gaia.py
test_cleanup_gaia (test_cleanup_gaia.TestCleanupGaia) ... FAIL

======================================================================
FAIL: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bsilverberg/.virtualenvs/gaia/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/marionette_test.py", line 163, in run
    testMethod()
  File "/Users/bsilverberg/gitRepos/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cleanup_gaia.py", line 51, in test_cleanup_gaia
    self.check_initial_state()
  File "/Users/bsilverberg/gitRepos/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cleanup_gaia.py", line 67, in check_initial_state
    self.assertEqual(self.data_layer.media_files, [])
AssertionError: Lists differ: [u'/sdcard/IMG_0001_1.jpg', u'... != []

First list contains 3 additional elements.
First extra element 0:
/sdcard/IMG_0001_1.jpg

+ []
- [u'/sdcard/IMG_0001_1.jpg',
-  u'/sdcard/IMG_0001_2.jpg',
TEST-UNEXPECTED-FAIL | test_cleanup_gaia.py test_cleanup_gaia.TestCleanupGaia.test_cleanup_gaia | -  u'/sdcard/IMG_0001_3.jpg']
----------------------------------------------------------------------
Ran 1 test in 51.802s

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8430740 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19764

Test passes on both Flame and Hamachi

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks guys for running those tests - it's really appreciated. The Travis-CI run failed because the test_cleanup_gaia unit test is no longer safe to run on desktop B2G. I've spent some more time thinking about this change and I've decided I'm really not comfortable having code that could potentially irrevocably remove files from users systems without their knowledge or consent. Admittedly something would need to go wrong for this code path to execute again B2G desktop, but I'm still uneasy about it.

On IRC yesterday Zac pointed out that a good method to wipe out the storage locations should make the targeted removal of media files unnecessary. This was achieve previously by making the device root equal to the single storage location. We potentially have multiple storage locations, but we can certainly start with the primary one and expand later to cover more - perhaps with a mozdevice enhancement to determine them.

This however does not help b2gpopulate, which attempts to remove all files of a certain type before pushing new content. We could keep these delete_all_ methods in gaiatest and use them from b2gpopulate, but there's always the risk they'd be used by others and could result in lost files. We could implement the deletion directly into b2gpopulate, but then if it's run against a desktop B2G build there's the risk again of dataloss. I'm keen to hear what you guys think.

Comment 11

[Zac C (:zac)]

I feel that it's not necessary to keep the use of the methods in GaiaTestCase.setUp() which removes a large element of the risk.

Which just leaves the b2gpopulate solution to be decided.

I'm OK with leaving the atoms in gaiatest but unused (except for by external packages). There are lots of other methods and features that can do naughty things to your device/computer and I think that comes with the territory and is acknowledged when the user does so in their testvars file.

That said, could you protect them further so that they only run when it's a device or fake-sdcard is set?

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #11)
> I feel that it's not necessary to keep the use of the methods in
> GaiaTestCase.setUp() which removes a large element of the risk.
> 
> Which just leaves the b2gpopulate solution to be decided.
> 
> I'm OK with leaving the atoms in gaiatest but unused (except for by external
> packages). There are lots of other methods and features that can do naughty
> things to your device/computer and I think that comes with the territory and
> is acknowledged when the user does so in their testvars file.

b2gpopulate doesn't have the same acknowledgement of risks, though perhaps it should.

> That said, could you protect them further so that they only run when it's a
> device or fake-sdcard is set?

At the moment these methods are in GaiaData, which does not have access to GaiaDevice so it's a bit of a pain to determine if we're running on a desktop B2G build. We can do that from b2gpopulate though.

Okay, so I think I'll update this patch to remove the use of these methods from cleanup_gaia, but keep the methods for use by b2gpopulate. I'll think about how I can make b2gpopulate as safe to use as possible, but it will be outside the scope of this bug. Still keen to hear Bob's thoughts too.

Comment 13

[Bob Silverberg [:bsilverberg]]

I'm in agreement with what's been said. I am also uncomfortable having code that could potentially wipe files from a user's computer, so I agree that we should not have that code. Other than that, whatever solution works for now is a good one, I think. If we encounter cases in the future where it doesn't work we can address those as they come up. Perfect is the enemy of good.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Unfortunately this doesn't work for b2gpopulate because the delete runs in the context of the current app, and even the System app doesn't have permission to delete music files. This was discovered after writing unit tests for the changes. I've raised bug 1020216 to address deleting the media files in b2gpopulate. If you're interested, I'll leave my branch with my latest changes up for a while: https://github.com/davehunt/gaia/compare/bug1017521