Ignore ios-unrelated test onDensityChange_shouldUpdateFlingBehavior #1826
Conversation
| @@ -2477,7 +2478,7 @@ class ScrollableTest { | |||
| } | |||
|
|
|||
| @Test | |||
| @Ignore // TODO(https://youtrack.jetbrains.com/issue/CMP-7220) Fails on iOS | |||
| @IgnoreUIKitTarget // Density not taken into account in CupertinoFlingBehaviour | |||
There was a problem hiding this comment.
It doesn't seem right to be dependent on compilation target here. As far as I remember CupertinoFlingBehaviour doesn't use platform API and might be reused in web for iOS for example.
There was a problem hiding this comment.
The test was built around the DefaultFlingBehavior, but treated as a skiko scroll test. Considering that it was copy-pasted from Android tests, it looks like modifying it is not the goal of the ticket. In this case, ignoring it is the way to go.
There was a problem hiding this comment.
Could you please clarify WHY CupertinoFlingBehaviour shouldn't take density into account?
There was a problem hiding this comment.
CupertinoFlingBehaviour uses own decay spec (see CupertinoScrollDecayAnimationSpec), which isn't relay on density - I didn't implement this algorithm, do I cannot be 100% sure.
As an assumptions - decelerations and bounces are derived from user interaction gestures, which are based on screen pixels by their nature. The number of pixels per second of the User's gesture will be the same, no matter which DP used, hence, decelerations also should be the the same. And this assumption was confirmed by my experiments with native iOS scrolls.
The only value that can be taken into account in current scenario is screen pixels density, but it's an another topic.
There was a problem hiding this comment.
There are a few things that are kind of confusing/concerning:
- Ignoring test based on compilation target instead of testing different implementations by different tests. For example for the long term I expect reusing this implementation for Web on iOS devices.
- The fact that it's "by design" only for iOS. You reasoning is applicable on other platforms
- The fact that it's broken after AOSP merge and NOT by introducing this implementation in our fork in the first place. At least it means that current tests are checking what they are not supposed to
Currently I'm very sceptical that it should be done in this way. Let's hear some feedback from other teammates
There was a problem hiding this comment.
I don't find any confuses. Currently, Cupertino is designed and implemented for iOS only. Moving to another platforms will require some adoption anyway.
You're right that the test is not testing what is should test. My point here is that the test should test the DefaultFlingBehavior, not the general fling.
The test is not relevant for iOS because:
Fixes https://youtrack.jetbrains.com/issue/CMP-7220/Fix-onDensityChangeshouldUpdateFlingBehavior-test