1456520 - testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py TestCapabilities.test_additional_capabilities | AssertionError: u'/storage/sdcard/tests/profile' != '/sdcard/tests/profile' with adb.py

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Geoff Brown [:gbrown]]

When I convert Android Marionette tests to use adb.py instead of devicemanager, one test fails:

https://treeherder.mozilla.org/logviewer.html#?job_id=175209487&repo=try&lineNumber=1874

[task 2018-04-23T23:19:41.157Z] 23:19:41  WARNING -  TEST-UNEXPECTED-FAIL | testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py TestCapabilities.test_additional_capabilities | AssertionError: u'/storage/sdcard/tests/profile' != '/sdcard/tests/profile'
[task 2018-04-23T23:19:41.157Z] 23:19:41     INFO -  Traceback (most recent call last):
[task 2018-04-23T23:19:41.159Z] 23:19:41     INFO -    File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 159, in run
[task 2018-04-23T23:19:41.159Z] 23:19:41     INFO -      testMethod()
[task 2018-04-23T23:19:41.159Z] 23:19:41     INFO -    File "/builds/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py", line 62, in test_additional_capabilities
[task 2018-04-23T23:19:41.160Z] 23:19:41     INFO -      self.assertEqual(self.caps["moz:profile"].lower(), current_profile.lower())
[task 2018-04-23T23:19:41.160Z] 23:19:41     INFO -  TEST-INFO took 18899ms

Comment 1

[Geoff Brown [:gbrown]]

The difference between adb.py and devicemanager goes back to the test root. 

On the 4.3 emulator, /sdcard is a link to /storage/sdcard and devicemanagerADB.getDeviceRoot() returns '/storage/sdcard/tests' while ADBDevice.test_root is '/sdcard/tests'.

Comment 2

[Geoff Brown [:gbrown]]

Looking at the test:

https://dxr.mozilla.org/mozilla-central/rev/dfb15917c057f17e5143f7d7c6e1972ba53efc49/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py#62

  self.assertEqual(self.caps["moz:profile"].lower(), current_profile.lower())

current_profile starts with ADBDevice.test_root.

I think self.caps["moz:profile"] comes from

https://dxr.mozilla.org/mozilla-central/rev/dfb15917c057f17e5143f7d7c6e1972ba53efc49/testing/marionette/session.js#378

  ["moz:profile", maybeProfile()],

https://dxr.mozilla.org/mozilla-central/rev/dfb15917c057f17e5143f7d7c6e1972ba53efc49/testing/marionette/session.js#545

  return Services.dirsvc.get("ProfD", Ci.nsIFile).path;

and that starts with '/storage/sdcard'.


:bc -- Thoughts?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Geoff, `self.caps["moz:profile"]` is the profile path as used internally by Firefox (like what you see in about:support).

Comment 4

[Bob Clary [:bc] (inactive)]

nexus one/cyanogenmod/sdk 10
$ adb -s HT9CPP809750 shell readlink /sdcard
/mnt/sdcard

gs3/lineageOS/sdk 25
$ adb -s 501a2aa7 shell readlink /sdcard
/storage/self/primary

if current_profile.startswith('/sdcard'):
    target = adb.shell_output("readlink /sdcard", ....)
    current_profile = current_profile.replace('/sdcard', target, 1)    
current_profile = current_profile.lower()

Comment 5

[Geoff Brown [:gbrown]]

Attachment: update test_additional_capabilities to allow for fennec profile on /sdcard

This is pretty hacky, but it works: on Android, when the mozdevice profile path does not match the firefox profile path, and if the root of the mozdevice profile path is a symlink, substitute the linked directory for the symlink. I had to resort to 'ls -l' to identify the symlink, since readlink is not available on sdk 18.

I am left wondering, should I instead skip this test on Android?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cab9a5fb53a18ab4d8196f21fa00dac2e03bbdfe

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8970710 [details] [diff] [review]
update test_additional_capabilities to allow for fennec profile on /sdcard

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

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
@@ +60,5 @@
> +            match = re.match(r'.*->\s(.*)', ls_out)
> +            if match:
> +                new_root = match.group(1)
> +                profile = profile.replace(root, new_root)
> +        return profile

For this test I would be happy if we could just do a check with `assertIn`. Wouldn't that work too in all the cases?

Comment 7

[Bob Clary [:bc] (inactive)]

assertIn wouldn't work in the case I mentioned in comment 4 would it?

gs3/lineageOS/sdk 25
$ adb -s 501a2aa7 shell readlink /sdcard
/storage/self/primary

Comment 8

[Bob Clary [:bc] (inactive)]

Comment on attachment 8970710 [details] [diff] [review]
update test_additional_capabilities to allow for fennec profile on /sdcard

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

wfm. The only other way I can think of is to create a dummy file using one path and retrieving it via the other.

I can see the potential for this test to catch issues, but it is unfortunate we have to do this. Henrik, have you ever seen this fail on Android? If not, maybe drop the test on Android.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Bob Clary [:bc:] from comment #7)
> assertIn wouldn't work in the case I mentioned in comment 4 would it?
> 
> gs3/lineageOS/sdk 25
> $ adb -s 501a2aa7 shell readlink /sdcard
> /storage/self/primary

What does Firefox report as profile path in that case?

Comment 10

[Bob Clary [:bc] (inactive)]

Not sure how to get the value the framework gives. Logcat shows paths with /sdcard not /storage. Marionette crashes.

Comment 11

[Geoff Brown [:gbrown]]

It is kind-of reassuring to have a test that verifies that fennec is actually using the same profile directory as specified by the harness with -profile, and I doubt we test for that anywhere else: let's keep the test.

I imagine there are other cases like comment 7 where an assertIn solution would give us trouble: for whatever reason,  /sdcard's physical location seems to move around to various odd places on different devices.

I'd like to go ahead with the existing patch, as a least-worst solution. whimboo?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8970710 [details] [diff] [review]
update test_additional_capabilities to allow for fennec profile on /sdcard

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

r=wc addressed.

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
@@ +47,4 @@
>      def test_supported_features(self):
>          self.assertIn("rotatable", self.caps)
>  
> +    def get_fennec_profile(self):

Please move this helper before `test_mandated_capabilities`.

@@ +54,5 @@
> +            # universally available (missing from sdk 18)
> +            import posixpath
> +            import re
> +            device = self.marionette.instance.runner.device.app_ctx.device
> +            root = posixpath.sep.join(profile.split(posixpath.sep)[0:2])

Please add an example here what specifically is getting changed for the path.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1c3d342559
Make test_capabilities.py more resilient to mozdevice changes; r=bc,whimboo

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9d1c3d342559