[Bug] Fix crash and add unit test to cover the issue. #1080
Conversation
aztec/build.gradle
Outdated
| testImplementation "org.mockito:mockito-inline:$mockitoVersion" | ||
| testImplementation "org.mockito.kotlin:mockito-kotlin:$mockitoKotlinVersion" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Awesome! Thanks for adding tests! 🙇🏼
| @VisibleForTesting | ||
| internal fun safelyParseColor(colorString: String?): Int? { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 I missed safelyParseColor. We'd need to in the end expose textColorValue, which in essence is equivalent to exposing the method safelyParseColor.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.
There was a problem hiding this comment.
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! 🙇🏼
|
@thomashorta ready for re-review 🙏🏻 |
There was a problem hiding this comment.
Thanks for introducing MockK! 🙇🏼
I added a few small comments and also a bigger response to a comment where I say that I believe testing the changes can be accomplished in a white-box manner, without the need of mocking the test subject itself.
But since this change is small enough and the code works and adds tests to the change, I will mark it as approved if you think it's better to move on with the merge soon rather than figuring out nitpicky stuff about the testing part. 😅
Thanks again for the fix!
|
@thomashorta I responded to your bigger point. But I made slightly bigger changes in response. As it turns out, I didn't even need to mock anything 🤦🏻 . Since we really only care about the crashes, this makes the most sense. Appreciate you elaborating as it helped me noticed an oversight I made. |
Fix
Fixes: wordpress-mobile/WordPress-Android#20738
Fix a crash that occurs when a gutenberg post is being edited and there is a block with text color on it, it's possible for the color to be malformed or not parselable. The reasoning for how the Color is empty is not clear, but the function is clear that an empty string is possible. Added unit test to also confirm this covers both an empty and null string color.
Test
MarkSpanTestpasses.Review
@thomashorta or @antonis
Make sure strings will be translated:
strings.xmlas a part of the integration PR.