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.

Sign up for GitHub

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

[Bug] Fix crash and add unit test to cover the issue. #1080

Merged
merged 4 commits into from
May 6, 2024
Merged

[Bug] Fix crash and add unit test to cover the issue. #1080

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5838fb5
[Bug] Fix crash and add unit test to cover the issue.
notandyvee May 1, 2024
8e29d07
[Bug] Remove mockito and used mockk instead.
notandyvee May 3, 2024
60e53c2
PR review round two.
notandyvee May 6, 2024
2a2a1d5
Removed uneccessary code.
notandyvee May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions aztec/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ dependencies {
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0', {
exclude group: 'com.android.support', module: 'support-annotations'
}
testImplementation "org.mockito:mockito-inline:$mockitoVersion"
testImplementation "org.mockito.kotlin:mockito-kotlin:$mockitoKotlinVersion"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Since it's the first time adding a mocking framework to this project, I would advocate adding MockK, what do you think?

I recently posted about Mockito issues and moving to MockK for Kotlin testing (pctCYC-1iC-p2) and the overall feedback was positive, so this feels like a nice opportunity here to avoid introducing Mockito in this Kotlin-heavy codebase if you're comfortable with that.


implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$kotlinCoroutinesVersion"
implementation "org.jetbrains.kotlinx:kotlinx-coroutines-android:$kotlinCoroutinesVersion"
Expand Down
6 changes: 4 additions & 2 deletions aztec/src/main/kotlin/org/wordpress/aztec/spans/MarkSpan.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.wordpress.aztec.spans
import android.graphics.Color
import android.text.TextPaint
import android.text.style.CharacterStyle
import androidx.annotation.VisibleForTesting
import org.wordpress.aztec.AztecAttributes
import org.wordpress.aztec.source.CssStyleFormatter

Expand All @@ -25,8 +26,9 @@ class MarkSpan : CharacterStyle, IAztecInlineSpan {
textColorValue = safelyParseColor(colorString)
}

private fun safelyParseColor(colorString: String?): Int? {
if (colorString == null) {
@VisibleForTesting
internal fun safelyParseColor(colorString: String?): Int? {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this doesn't need to be internal to be testable, since the public constructor on line 24 already calls safelyParseColor on a colorString passed as argument.

I didn't check the code further so I don't know if the super does something that's not supported at runtime, so if that's the case, feel free to ignore this comment. Otherwise I believe this could still be private and the unit test can just instantiate MarkSpan with empty and null strings to test the fixed scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this doesn't need to be internal to be testable

I think there is a misunderstanding. I marked it internal to decrease the visibility. Went from private to internal. Protected wouldn't work and I need it visible for testing. If you think it being public is fine, I can do that too. Or maybe there is a mockK feature that gets around this annoyance, let me know.

Copy link
Contributor

@thomashorta thomashorta May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What I meant is that even keeping this private, which I agree we should opt for the most restrict visibility we can here, you can test this method by calling the constructor of MarkSpan that receives a color String as an argument, then check in that created instance if getTextColor returns what you expect + the constructor doesn't throw any exceptions.

In other words, treating the class as a black box and testing behaviors, inputs, and outputs, instead of making a specific method visible for testing and white-box testing its source code.

Besides that, I think I anticipated myself a bit in my first comment mentioning super(), but what I meant is that I see that this class is a subclass and I didn't check the constructor of the superclass to affirm that calling the constructor of MarkSpan from a test can cause any issues, in which case it could make sense to bump the visibility to internal so we can at least test something.

Copy link
Contributor Author

@notandyvee notandyvee May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Great point. And I think we may be getting into the weeds here. What I did was on purpose and I specifically meant to test just a method.

When I approach unit tests I try and test a very specific unit of code. What if the implementation of one of the constructors changes? For example adding a new required parameter? The test could break making it brittle. If we test a specific method, it's independent of any business logic changes since we are testing just a function. And more importantly, we really only care about the color parsing which is what this crash is about.

Additionally we would no longer have the ability to check the return type of safelyParseColor. We'd need to in the end expose textColorValue, which in essence is equivalent to exposing the method safelyParseColor. I missed getTextColor. And that makes sense.

With all that said, it sounds like you feel strongly about running the test a specific way so I made the changes that I think you wanted. I've never used MockK, so let me know if there are any further changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I approach unit tests I try and test a very specific unit of code. What if the implementation of one of the constructors changes? For example adding a new required parameter? The test could break making it brittle. If we test a specific method, it's independent of any business logic changes since we are testing just a function. And more importantly, we really only care about the color parsing which is what this crash is about.

This is a valid point as well!

I tend to approach test with the class usage in mind, treating everything private as implementation details, which are subject to change at any time. Or as many say: "test behavior, not implementation". The testable "units" for me are any public APIs an external code can use to interact with the class.

For instance, in this situation, in the app perspective, the crash happened when instantiating MarkSpan (which in turn calls private/internal methods to initialize and set up itself). The calling code only interacts with the constructor and with the getTextColor public method, so it makes sense to call and assert using only them.

But what you said is totally valid as well, one might be more interested that each function inside a class work as expected, especially if they are more like scoped utility functions that are used several places inside that class. And that works fine especially in lib code (like this) and modularized projects, where internal helps with keeping things private to external callers.

Anyway, I think this was a nice discussion and I personally think the code looks cleaner now in this last iteration, so I will just reiterate my approval for this last code review.

Thanks for the changes! 🙇🏼

if (colorString.isNullOrBlank()) {
return null
}
return try {
Expand Down
52 changes: 52 additions & 0 deletions aztec/src/test/kotlin/org/wordpress/aztec/MarkSpanTest.kt
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks for adding tests! 🙇🏼

Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package org.wordpress.aztec

import org.junit.Assert
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mock
import org.mockito.Mockito
import org.robolectric.RobolectricTestRunner
import org.wordpress.aztec.spans.MarkSpan

@RunWith(RobolectricTestRunner::class)
class MarkSpanTest {
@Mock
val markSpan = Mockito.mock(MarkSpan::class.java)
/**
* Test used to confirm two crashes related are fixed.
*
* https://github.com/wordpress-mobile/WordPress-Android/issues/20738
*/
@Test
fun `Calling MarkSpan#safelyParseColor with empty string should not cause a crash`() {
var error = false
var result: Int? = null
try {
Mockito.`when`(markSpan.safelyParseColor("")).thenCallRealMethod()
result = markSpan.safelyParseColor("")
} catch (e: Exception) {
error = true
}
Assert.assertFalse(error)
Assert.assertEquals(null, result)
}

/**
* Test used to confirm two crashes related are fixed.
*
* https://github.com/wordpress-mobile/WordPress-Android/issues/20694
*/
@Test
fun `Calling MarkSpan#safelyParseColor with null string should not cause a crash`() {
var error = false
var result: Int? = null
try {
Mockito.`when`(markSpan.safelyParseColor(null)).thenCallRealMethod()
result = markSpan.safelyParseColor(null)
} catch (e: Exception) {
error = true
}
Assert.assertFalse(error)
Assert.assertEquals(null, result)
}
}
thomashorta marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ ext {
jSoupVersion = '1.11.3'
wordpressUtilsVersion = '3.5.0'
espressoVersion = '3.0.1'
mockitoVersion = '4.5.1'
mockitoKotlinVersion = '4.1.0'

// other
wordpressLintVersion = '2.0.0'
Expand Down
Loading