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

Improve SeekBarMapper #2037

Merged
merged 1 commit into from
May 17, 2024
Merged

Improve SeekBarMapper #2037

merged 1 commit into from
May 17, 2024

Conversation

xgouchet
Copy link
Member

What does this PR do?

Improve SeekBarMapper and its unit tests:

  • Better support for padding
  • Better support for thumb size
  • Include background if any
  • More readable tests

@xgouchet xgouchet requested review from a team as code owners May 16, 2024 14:12
@@ -151,6 +142,7 @@ internal abstract class BaseSeekBarWireframeMapperTest {
@Mock
lateinit var mockInternalLogger: InternalLogger

/*
Copy link
Member

Choose a reason for hiding this comment

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

should it be left as commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, this code should be deleted

Comment on lines 146 to 148
// region Assertions
fun assertThatBoundsAreCloseEnough(actual: MobileSegment.Wireframe, expected: MobileSegment.Wireframe) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// region Assertions
fun assertThatBoundsAreCloseEnough(actual: MobileSegment.Wireframe, expected: MobileSegment.Wireframe) {
// region Assertions
fun assertThatBoundsAreCloseEnough(actual: MobileSegment.Wireframe, expected: MobileSegment.Wireframe) {

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

if (actualCornerRadius != null && expectedCornerRadius != null) {
assertThat(actualCornerRadius).isCloseTo(expectedCornerRadius, withinPercentage(5))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

// endregion is missing, it seems

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@ForgeConfiguration(ForgeConfigurator::class)
internal class LegacySeekBarWireframeMapperTest : BaseSeekBarWireframeMapperTest() {

/*
Copy link
Member

Choose a reason for hiding this comment

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

should it all be commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same, it's deleted

@xgouchet xgouchet force-pushed the xgouchet/sr_ut_seekbar branch from 9a88e51 to e086414 Compare May 16, 2024 14:55
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

Attention: Patch coverage is 79.34783% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 83.17%. Comparing base (a814f9f) to head (a23e0e0).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2037      +/-   ##
===========================================
+ Coverage    82.93%   83.17%   +0.24%     
===========================================
  Files          491      491              
  Lines        17700    17697       -3     
  Branches      2683     2686       +3     
===========================================
+ Hits         14678    14718      +40     
+ Misses        2285     2238      -47     
- Partials       737      741       +4     
Files Coverage Δ
...nreplay/material/internal/SliderWireframeMapper.kt 97.40% <ø> (ø)
.../sessionreplay/internal/DefaultRecorderProvider.kt 92.31% <100.00%> (+6.59%) ⬆️
...eplay/internal/recorder/mapper/BasePickerMapper.kt 100.00% <ø> (ø)
...nternal/recorder/mapper/CheckableTextViewMapper.kt 96.55% <ø> (+39.66%) ⬆️
.../internal/recorder/mapper/CheckedTextViewMapper.kt 95.65% <ø> (ø)
...lay/internal/recorder/mapper/NumberPickerMapper.kt 99.19% <ø> (ø)
...lay/internal/recorder/mapper/SwitchCompatMapper.kt 88.37% <ø> (+17.05%) ⬆️
.../android/sessionreplay/internal/utils/MiscUtils.kt 72.73% <ø> (+1.82%) ⬆️
...ssionreplay/recorder/mapper/BaseWireframeMapper.kt 100.00% <ø> (ø)
...id/sessionreplay/recorder/mapper/TextViewMapper.kt 90.91% <ø> (+1.33%) ⬆️
... and 3 more

... and 21 files with indirect coverage changes

@xgouchet xgouchet requested a review from 0xnm May 16, 2024 15:42
@xgouchet xgouchet force-pushed the xgouchet/sr_ut_seekbar branch from e086414 to dadfe16 Compare May 16, 2024 15:42
@xgouchet xgouchet force-pushed the xgouchet/sr_ut_seekbar branch from dadfe16 to 5459252 Compare May 17, 2024 07:09
const val OPAQUE_ALPHA_VALUE: Int = MAX_ALPHA_VALUE

/** The value corresponding to a 25% opaque Alpha. */
const val PARTIALLY_OPAQUE_ALPHA_VALUE: Int = 0x44
Copy link
Member

@ambushwork ambushwork May 17, 2024

Choose a reason for hiding this comment

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

a random mathematical comment here, 0x3f or 0x40 seems closer to 25% of opaque alpha.(Just wondering how 0x44 is calculated, although it has hardly any impact)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point thanks

@xgouchet xgouchet force-pushed the xgouchet/sr_ut_seekbar branch from 5459252 to a23e0e0 Compare May 17, 2024 09:31
@xgouchet xgouchet merged commit a0e05c3 into develop May 17, 2024
21 checks passed
@xgouchet xgouchet deleted the xgouchet/sr_ut_seekbar branch May 17, 2024 13:31
@xgouchet xgouchet added this to the 2.10.x milestone Jul 31, 2024
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.

4 participants