square / spoon

Distributing instrumentation tests to all your Androids.
https://square.github.io/spoon/
Apache License 2.0
2.7k stars 477 forks source link

[Feature Request] Grant all permissions to test apk too #537

Closed tir38 closed 6 years ago

tir38 commented 6 years ago

What: The -grant-all flag grants all permissions to apk under test. It would be great if this flag granted all permissions to the test apk as well.

Reason: Sometimes the test apk needs permissions that the apk under test does not. This is true especially with Spoon which needs android.permission.WRITE_EXTERNAL_STORAGE. If the app itself doesn't need this permission, then the right thing to do is put that permission in the androidTest/AndroidManifest.xml. However there is no way to grant this permission during test.

A more detailed explanation of the problem is here

Solution: If I understand SpoonDeviceRunner correctly, we should be able to just duplicate the -g addition for the test apk:

https://github.com/square/spoon/blob/1e6674f3a0c4d896b68ac789d4f71a6a09646d0f/spoon-runner/src/main/java/com/squareup/spoon/SpoonDeviceRunner.java#L161

    // Now install the main application and the instrumentation application.
    for (File otherApk : otherApks) {
      try {
        String extraArgument = "";
        if (grantAll && deviceDetails.getApiLevel() >= DeviceDetails.MARSHMALLOW_API_LEVEL) {
          extraArgument = "-g";
        }
        device.installPackage(otherApk.getAbsolutePath(), true, extraArgument);
      } catch (InstallException e) {
        logInfo("InstallException while install other apk on device [%s]", serial);
        e.printStackTrace(System.out);
        return result.markInstallAsFailed("Unable to install other APK.").addException(e).build();
      }
    }
    try {
        String extraArgument = "";
        if (grantAll && deviceDetails.getApiLevel() >= DeviceDetails.MARSHMALLOW_API_LEVEL) {
          extraArgument = "-g";
        }
        device.installPackage(testApk.getAbsolutePath(), true, extraArgument);
    } catch (InstallException e) {
      logInfo("InstallException while install test apk on device [%s]", serial);
      e.printStackTrace(System.out);
      return result.markInstallAsFailed("Unable to install instrumentation APK.")
          .addException(e)
          .build();
    }
edenman commented 6 years ago

Sounds reasonable. Make a PR?

tir38 commented 6 years ago

will do.

tir38 commented 6 years ago

My initial plan was to extract

    /**
     * @param deviceDetails details for current device
     * @return optional argument for grantAll permissions, or empty string if not required
     */
    private String getGrantAllExtraArgument(DeviceDetails deviceDetails) {
        String extraArgument = "";
        if (grantAll && deviceDetails.getApiLevel() >= DeviceDetails.MARSHMALLOW_API_LEVEL) {
            extraArgument = "-g";
        }
        return extraArgument;
    }

and then call it twice:

  // Now install the main application and the instrumentation application.
    for (File otherApk : otherApks) {
        try {
            String extraArgument = getGrantAllExtraArgument(deviceDetails);
            device.installPackage(otherApk.getAbsolutePath(), true, extraArgument);
             ...
    }
    try {
        String extraArgument = getGrantAllExtraArgument(deviceDetails);
        device.installPackage(testApk.getAbsolutePath(), true, extraArgument);
        ...
    }

This solves the general problem of granting all permissions to the test apk. Then I saw this method

private void grantReadWriteExternalStorage(DeviceDetails deviceDetails, IDevice device)

which gets called automatically (regardless of -grantAll). Would I be better off updating this method instead to grant read/write permission to test apk? This would solve my specific problem of granting read/write perms.

edenman commented 6 years ago

I'd lean toward just applying the -g flag to both

edenman commented 6 years ago

Thanks!

tir38 commented 6 years ago

I assume this will go out with the next SNAPSHOT?

edenman commented 6 years ago

yup. should be automatically created if it's not already

tir38 commented 6 years ago

This is now technically working (test apk gets granted permissions) but I'm no longer seeing screenshots in my test report.

tir38 commented 6 years ago

Ahh Spoon still uses the target context to create the directory:

File screenshotDirectory =
        obtainDirectory(activity.getApplicationContext(), className, methodName, SPOON_SCREENSHOTS);

So I guess the target context has to have the WRITE_STORAGE permission.

tir38 commented 6 years ago

I guess this is a separate issue. I'll open another ticket if I need to.

tir38 commented 6 years ago

TL;DR grantAll for the test apk surely has some benefit, but not if all you want to do is move the WRITE_STORAGE_PERMISSION into your test apk's manifest (https://jasonatwood.io/archives/1784)