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

Remove deprecated targetSdk property in library modules #145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JolandaVerhoef
Copy link
Collaborator

As per https://stackoverflow.com/questions/75256272/kotlin-multiplatform-mobile-targetsdk-deprecated, the targetSdk property is not needed in library modules. This PR removes them.

@JolandaVerhoef
Copy link
Collaborator Author

@Zeronfinity One of the unit tests is failing (both locally and on Github) because it times out waiting for PixelCopy to complete. Any chance you know what could cause this?

@Zeronfinity
Copy link
Collaborator

Zeronfinity commented Mar 25, 2024

Hi @JolandaVerhoef . It seems like - before the change, Robolectric is running the tests for only the target SDK version, which was 34. When the target SDK version is removed, Robolectric is running the tests for min SDK version only, which is 21. Meanwhile, it seems like ShadowPixelCopy is supported for SDK >= 26.

Please add a minSdk annotation to the problematic test to solve this issue, i.e. change @Config(shadows = [ShadowPixelCopy::class]) to @Config(shadows = [ShadowPixelCopy::class], minSdk = 26) for test screenFlashOverlay_fullWhiteWhenEnabled().

While the minSdk should definitely be added with ShadowPixelCopy test, I am not sure if this change is good with Robolectric overall. If Robolectric is being run on only one SDK level, shouldn't running on the target API level be quite important? Ideally, Robolectric should be configured to test on all SDK levels from min SDK and onwards (this does happen when the minSdk annotation is added to robolectric tests). But I guess this is something to discuss with JCA team, exactly what behavior we should try to use for the robolectric tests.

And another thing I am curious about, it seems like the target SDK level is being deprecated because it's just a hint. But this hint may already being used by various libraries, e.g. Robolectric in this case (but there might be other cases too in future). So in a bottom-up approach (e.g. running a module independently), any dependency on the target SDK level will face problem. That makes me wonder if removing it from the modules is really important.

@Zeronfinity
Copy link
Collaborator

By the way, while testing it out, it seems like adding a min SDK makes the tests eventually fail at later SDKs due to heap space error. Quick guess is it might be an issue with compose resources not being properly cleaned up (or reused) after Robolectric runs a test for each of the SDK levels.

@temcguir temcguir requested review from temcguir and Zeronfinity and removed request for temcguir March 25, 2024 18:24
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.

3 participants