Skip to content

Improve SeekBarMapper#2037

Merged
xgouchet merged 1 commit into
developfrom
xgouchet/sr_ut_seekbar
May 17, 2024
Merged

Improve SeekBarMapper#2037
xgouchet merged 1 commit into
developfrom
xgouchet/sr_ut_seekbar

Conversation

@xgouchet

Copy link
Copy Markdown
Contributor

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
@Mock
lateinit var mockInternalLogger: InternalLogger

/*

Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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

codecov-commenter commented May 16, 2024

Copy link
Copy Markdown

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

@ambushwork ambushwork May 17, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor 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
@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