Skip to content

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
Contributor

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
Contributor

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 Zeronfinity and temcguir 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