Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only using AttributionSource for compile sdk 31 and above #6884

Conversation

utzcoz
Copy link
Member

@utzcoz utzcoz commented Nov 24, 2021

Fixes #6883.

Signed-off-by: utzcoz <utzcoz@outlook.com>
@utzcoz
Copy link
Member Author

utzcoz commented Nov 24, 2021

Also testing it with example provided #6883 and Robolectric local SNAPSHOT.

@utzcoz utzcoz requested review from dmeng and hoisie November 24, 2021 16:44
@utzcoz
Copy link
Member Author

utzcoz commented Nov 24, 2021

Looks like it breaks ShadowAppOpsManagerTest. I will check it.

@utzcoz utzcoz changed the title Only using attribution source for compile sdk 31 and above Only using AttributionSource for compile sdk 31 and above Nov 24, 2021
@utzcoz
Copy link
Member Author

utzcoz commented Nov 24, 2021

Looks like it uses real noteProxyOpNoThrow with AttributionSource input instead of changed method with Object type for AttributionSource.

@utzcoz
Copy link
Member Author

utzcoz commented Nov 25, 2021

What about only loading methods and related classses supported by current running SDK? For example, if we only run tests with SDK 30, we will not load methods needs SDK 31 and above. But I remember there are some methods have been mixed with other purpose, for example normal API function invoked by other methods supported by other APIs.

@utzcoz utzcoz force-pushed the only-using-AttributionSource-for-compile-SDK-31-and-above branch from ae7c996 to 35d4bab Compare November 25, 2021 16:54
@hoisie
Copy link
Contributor

hoisie commented Nov 27, 2021

Can you squash the last 2 commits so ShadowAppOpsManagerTest is not broken in a commit.

Copy link
Contributor

@hoisie hoisie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Only one minor thing -- squash the last 2 commits.

@hoisie
Copy link
Contributor

hoisie commented Nov 27, 2021

FWIW, long-term, I think there could be better alternatives to looseSignatures . For instance, if shadows supported reflector's WithType, that method could be:

@Implementation(minSdk = Build.VERSION_CODES.S)
  protected int noteProxyOpNoThrow(
      int op,
      @WithType("android.content.AttributionSource") Object attributionSource,
      String message,
      boolean ignoredSkipProxyOperation) { ... }

The advantage of this is that you don't have to worry about casting all of the types, only the problemetic ones.

We have tried to use Object to replace AttributionSource as type of
second input parameter for noteProxyOpNoThrow(int, AttributeSource,
String, bool) to make sure ShadowAppOpsManager can be loaded and used
when compile SDK is less than 31. But changed method's siganture doesn't
match origin method signature, and it will be used as shadow method. To
fix this problem, this CL uses looseSignatures for ShadowAppOpsManager,
and chagnes all input parameters' type to Object to meet
looseSignatures' requirement.

Signed-off-by: utzcoz <utzcoz@outlook.com>
@utzcoz utzcoz force-pushed the only-using-AttributionSource-for-compile-SDK-31-and-above branch from 35d4bab to bb36cda Compare November 27, 2021 05:53
Signed-off-by: utzcoz <utzcoz@outlook.com>
@utzcoz
Copy link
Member Author

utzcoz commented Nov 27, 2021

FWIW, long-term, I think there could be better alternatives to looseSignatures . For instance, if shadows supported reflector's WithType, that method could be:

@Implementation(minSdk = Build.VERSION_CODES.S)
  protected int noteProxyOpNoThrow(
      int op,
      @WithType("android.content.AttributionSource") Object attributionSource,
      String message,
      boolean ignoredSkipProxyOperation) { ... }

The advantage of this is that you don't have to worry about casting all of the types, only the problemetic ones.

I prefer this method more than current looseSignatures. It only needs to change needed input parameters, not all of one method. I will fire an issue to track it.

@utzcoz
Copy link
Member Author

utzcoz commented Nov 27, 2021

Thanks for doing this! Only one minor thing -- squash the last 2 commits.

Done.

@hoisie hoisie merged commit 2ca5469 into robolectric:master Nov 27, 2021
@utzcoz utzcoz deleted the only-using-AttributionSource-for-compile-SDK-31-and-above branch November 27, 2021 15:21
@hoisie
Copy link
Contributor

hoisie commented Nov 28, 2021

I'll do a 4.7.3 release with this and #6880

@hoisie
Copy link
Contributor

hoisie commented Dec 1, 2021

This should now be fixed in 4.7.3 (may take a minute or two for that to appear on MavenCentral).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoClassDefFoundError with Robolectric 4.7.2, sdk 30 and ContentProvider tests
2 participants