999506 - Make GaiaDevice / sdcard interactions work with different device types.

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

Currently we assume the SD Card location is:
/sdcard/

On devices with multiple storage locations (eg Peak, Flame) it is:
/storage/sdcard0/   (internal storage)
/storage/sdcard1/   (removable sdcard)

We need to update a few snippets in gaia_test.py to detect the location and push/pull appropriately. The storage location in use must also be the one configured in the Gaia settings for the tests to see the files.

There may be some features in mozdevice we can use to find the location of the storage.

Comment 1

[Zac C (:zac)]

Ahal, wlach, should getDeviceRoot return a path to the sdcard on firefox OS devices?

The two examples in comment #0 are not covered, presently.

At the moment with mozdevice==0.33 the value returned for getDeviceRoot is /data/local/tests but we don't use the value. We work straight from /sdcard/.

Alternatively I just put this logic straight into the test framework itself by checking with dirExists or something like that.

Comment 2

[William Lachance (:wlach)]

We can make getDeviceRoot do whatever we want. :) I think we should make it return a path to the sdcard on firefox os devices, indeed. Do you need/want to be able select between removable and non-removable storage somehow?

(In reply to Zac C (:zac) from comment #1)
> Ahal, wlach, should getDeviceRoot return a path to the sdcard on firefox OS
> devices?
> 
> The two examples in comment #0 are not covered, presently.
> 
> At the moment with mozdevice==0.33 the value returned for getDeviceRoot is
> /data/local/tests but we don't use the value. We work straight from /sdcard/.
> 
> Alternatively I just put this logic straight into the test framework itself
> by checking with dirExists or something like that.

Comment 3

[William Lachance (:wlach)]

Ping on the above question.

Comment 4

[Zac C (:zac)]

Sure I know we can do it, but *should* we..! At present we just need a safe way to get the default location. mozdevice may need to make assumptions, whereas in the test framework can probe the device and its settings a bit more to get the real location perhaps.

At present we don't need to switch between removable and non-removable. If we did we would probably want to explicitly target that directory so we'd know the location of it.

Comment 5

[Zac C (:zac)]

Just to clarify, for "default location" I meant "default storage location" which should be non-removable if it exists, or removable if it does not. That's the assumption I'm concerned about; is it safe.
Anyway, should I file a bug against mozdevice for this feature?

Comment 6

[William Lachance (:wlach)]

Attachment: Add /sdcard to list of directories we search for a testroot in

(In reply to Zac C (:zac) from comment #4)
> Sure I know we can do it, but *should* we..! At present we just need a safe
> way to get the default location. mozdevice may need to make assumptions,
> whereas in the test framework can probe the device and its settings a bit
> more to get the real location perhaps.
> 
> At present we don't need to switch between removable and non-removable. If
> we did we would probably want to explicitly target that directory so we'd
> know the location of it.

Really getDeviceRoot() is a shortcut for finding some scratch space that various test harnesses can use to store stuff on the device. We should make it return a reasonable value on all platforms. I think a simple patch that does something like this should probably do the trick. Give it a try maybe?

Comment 7

[Zac C (:zac)]

Comment on attachment 8410402 [details] [diff] [review]
Add /sdcard to list of directories we search for a testroot in

For Firefox OS devices without internal storage:
'/sdcard'

for firefox OS devices with internal storage:
'/storage/sdcard0'
'/storage/sdcard1'

It's the latter we need to support, without blocking the former.

Also should this be above '/data/local' ?

Comment 8

[Andrew Halberstadt [:ahal]]

Does /data/local exist in devices with internal storage? If not then it will be ignored anyway, so doesn't really matter. If yes, then why can't we continue using that instead of /storage/sdcard0?

Sounds like the only ordering that's important here is that '/storage/sdcard0' and '/storage/sdcard1' appear before '/sdcard'

Comment 9

[[:fabrice] Fabrice Desré]

/data/local exists on all devices. This is where we store 3rd party apps, indexedDB data, etc.

Comment 10

[Zac C (:zac)]

I'll have to check tomorrow, but I'm not sure that if [for example] we push a jpg file to /data/local/tests the Gallery app will find the file there.

Comment 11

[Zac C (:zac)]

Ok just tested with Hamachi and Flame device.

The existing logic is OK on Hamachi because /mnt/sdcard maps to /sdcard (or vice versa). 

For the Flame /data/local exists, but if I push a file to it (/data/local/tests) the Gallery app won't detect it. I suppose the API does not scan this directory for media. Thus we need to push directly to one of the sdcards.

Comment 12

[Zac C (:zac)]

Testing using getDeviceRoot, I've struck a new problem here. "/mnt/sdcard" appears to be a symlink and we can push files to that fine, but when we find that file back using the WebAPI it returns the "/sdcard" path to the file so we get a few test failures.

Comment 13

[Zac C (:zac)]

Attachment: github pr

I did experiment using getDeviceRoot but because the location of the push changes (based on mozdevice's logic) there needs to be a change to the unit tests and to the atoms.

Thus I decided to experiment with the logic inside gaiatest for the time being, hence the f? This method requires minimal changes to the atoms and unit tests.

Of course we can still move this logic into mozdevice if it doesn't affect other projects. I really just wanted to see how blocked we were on the flame for now.

I tested this on both hamachi and flame.

Comment 14

[Andrew Halberstadt [:ahal]]

So changing the ordering to:

- /storage/sdcard0
- /storage/sdcard1
- /sdcard
- /mnt/sdcard
- /data/local

should work? Also note that there's a 'deviceRoot' option in the dm constructor that can be used to override this default.

Comment 15

[Zac C (:zac)]

That would work ahal. Would that affect other mozdevice users?

Comment 16

[Andrew Halberstadt [:ahal]]

I don't think so.. but it's possible. I wouldn't worry about it, if something does come up it will be easy to fix.

Comment 17

[Zac C (:zac)]

OK! I'm glad you can't edit bugzilla comments ;)

Comment 18

[Zac C (:zac)]

(In reply to Zac C (:zac) from comment #13)
> Created attachment 8410955 [details] [review]
> github pr
> 
> I did experiment using getDeviceRoot but because the location of the push
> changes (based on mozdevice's logic) there needs to be a change to the unit
> tests and to the atoms.
> 
> Thus I decided to experiment with the logic inside gaiatest for the time
> being, hence the f? This method requires minimal changes to the atoms and
> unit tests.
> 
> Of course we can still move this logic into mozdevice if it doesn't affect
> other projects. I really just wanted to see how blocked we were on the flame
> for now.
> 
> I tested this on both hamachi and flame.

I re-did this to take into account the changes ahal proposed in comment #14.

Obviously it won't work until a new mozdevice is released :)

Comment 19

[Zac C (:zac)]

Comment on attachment 8410955 [details] [review]
github pr

Dave, I had to change the atom to take into account the tests/ suffix that getDeviceRoot adds.

but it was due for an r? anyway.

It can wait until a new mozdevice is released.

Comment 20

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

Comment on attachment 8410955 [details] [review]
github pr

See outstanding request for clarification of the atom change.

Comment 21

[Zac C (:zac)]

Comment on attachment 8410955 [details] [review]
github pr

I've clarified the atom change in the Github comments.

Comment 22

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

Comment on attachment 8410955 [details] [review]
github pr

I'm still unsure about the atom change. It doesn't seem to be necessary for this bug, but so long as we test that b2gpopulate doesn't regress then it's okay with me.

Comment 23

[Dave Hylands [:dhylands]]

(In reply to Andrew Halberstadt [:ahal] (PTO until May 6th) from comment #14)
> So changing the ordering to:
> 
> - /storage/sdcard0
> - /storage/sdcard1
> - /sdcard
> - /mnt/sdcard
> - /data/local
> 
> should work? Also note that there's a 'deviceRoot' option in the dm
> constructor that can be used to override this default.

So, personally, I don't think that hardcoding paths is the right thing to do.

A couple of corrections from previous comments:
1 - /sdcard is almost always a symlink to the real sdcard volume location.
2 - /mnt/sdcard should always be a mount point and not a symlink.

On devices which have sharable sdcards, (i.e. sdcard is in a separate partition), then you should be able to do:

adb shell vdc volume list | grep sdcard

On devices like the hamachi, this will return something like this:

110 sdcard /mnt/sdcard 4

which tells us that /mnt/sdcard is the mount point. Some devices have sdcard and extsdcard volumes, so you'll probably need to refine the grep to make sure you're getting the right place. /storage/sdcard0 and /storage/sdcard1 is peak/keon specific, not generic. Helix and Leo both use different schemes from that and from each other IIRC.

On devices, like the Nexus-4, which have no sharable sdcard, you should probably just use /sdcard.

On the nexus4, /sdcard is a symlink to /storage/emulated/legacy and /storage/emulated/legacy is a symlink to /mnt/shell/emulated/0

Comment 24

[Dave Hylands [:dhylands]]

Oh yeah, /data/local is never a place that device storage would look for files, except if someone sets up a fake volume to allow that to happen (I've done this for testing, but it doesn't happen on normal phones).

Comment 25

[Zac C (:zac)]

:dhylands, those comments are probably better placed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1000918

As in this bug we agreed to cede the logic to mozdevice!

Comment 26

[Zac C (:zac)]

Comment on attachment 8410955 [details] [review]
github pr

Bebe and Bob, this pull should work seamlessly with Flame and Hamachi now that mozdevice 0.34 has been released.

Can you give it a r? thanks!

Comment 27

[Florin Strugariu [:Bebe]]

Comment on attachment 8410955 [details] [review]
github pr

this is failing on Flame





(z1)florinstrugariu@P5171:~/gaia/gaia/tests/python/gaia-ui-tests$ gaiatest --testvars=/home/florinstrugariu/testvars.json --address=localhost:2828 --type=b2g --timeout=10000 --html-output=results/index.htm --restart /home/florinstrugariu/gaia/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cleanup_sdcard.pystarting httpd
running webserver on http://192.168.77.192:48097/
TEST-START test_cleanup_sdcard.py
test_cleanup_scard (test_cleanup_sdcard.TestCleanupSDCard) ... FAIL

======================================================================
FAIL: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/florinstrugariu/.virtualenvs/z1/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/florinstrugariu/gaia/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cleanup_sdcard.py", line 11, in test_cleanup_scard
    self.assertEqual(len(self.data_layer.media_files), 0)
TEST-UNEXPECTED-FAIL | test_cleanup_sdcard.py test_cleanup_sdcard.TestCleanupSDCard.test_cleanup_scard | AssertionError: 11 != 0
----------------------------------------------------------------------
Ran 1 test in 29.737s

FAILED (failures=1)

SUMMARY
-------
passed: 0
failed: 1
todo: 0

FAILED TESTS
-------
test_cleanup_sdcard.py test_cleanup_sdcard.TestCleanupSDCard.test_cleanup_scard
mozversion.mozversion.RemoteB2GVersion INFO | Unable to find system/sources.xml
(z1)florinstrugariu@P5171:~/gaia/gaia/tests/python/gaia-ui-tests$ pip freeze
ManifestDestiny==0.6
argparse==1.2.1
gaiatest==0.23
marionette-client==0.7.7
marionette-transport==0.1
mozcrash==0.12
mozdevice==0.34
mozfile==1.1
mozhttpd==0.7
mozinfo==0.7
mozlog==1.7
moznetwork==0.24
mozprocess==0.19
mozprofile==0.21
mozrunner==5.36
moztest==0.3
mozversion==0.4
plivo==0.9.6
requests==2.2.1
wsgiref==0.1.2
yoctopuce==1.01.12553

Comment 28

[Zac C (:zac)]

Comment on attachment 8410955 [details] [review]
github pr

Bebe, re-filed it with some modifications. The mozdevice solution doesn't work perfectly because we need a quite high level use of the sdcard rather than just a folder on it. :( sorry I did not notice this use case earlier.

Comment 29

[Zac C (:zac)]

+sdcard adhoc run:
http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.hamachi.mozilla-central.ui.adhoc/394/

Comment 30

[Florin Strugariu [:Bebe]]

Comment on attachment 8410955 [details] [review]
github pr

Woks fine now

Comment 31

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8410955 [details] [review]
github pr

This is fine on my Hamachi, but fails on my Flame. I believe this is because I have an older Flame that is unable to see the sdcard after flashing gaia and gecko, so I don't think it's a valid failure of this patch.

So consider this an r+ for Hamachi only.

Comment 32

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/cf485fc000d9dae5ad545a3643ffc6a7eecb2d48

v1.4:
https://github.com/mozilla-b2g/gaia/commit/2f0c49e1561720628179d83a68b2abdec78abd51