Improve range split function and add unit tests #553
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
289Adam289
changed the title
289Adam289
changed the title
…e-split-add-unit-tests
android/src/test/java/com/expensify/livemarkdown/RangeSplitterTest.java
Outdated
Show resolved
Hide resolved
android/src/test/java/com/expensify/livemarkdown/RangeSplitterTest.java
Outdated
Show resolved
Hide resolved
android/src/test/java/com/expensify/livemarkdown/RangeSplitterTest.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/expensify/livemarkdown/RangeSplitter.java
Outdated
Show resolved
Hide resolved
android/src/test/java/com/expensify/livemarkdown/RangeSplitterTest.java
Outdated
Show resolved
Hide resolved
| private void testRange(MarkdownRange range, int start, int end, String type) { | ||
| assertEquals(start, range.getStart()); | ||
| assertEquals(end, range.getEnd()); | ||
| assertEquals(type, range.getType()); | ||
| } |
There was a problem hiding this comment.
Instead of implementing a custom testRange function (which btw should be named assertRangeStartEndType or something along these lines), let's just implement MarkdownRange#equals method and rewrite the assertions in the unit tests like:
assertEquals(new MarkdownRange("type", 0, 1, 0), markdownRanges.get(0));This way we get more readable logs (with expected and actual value) if test fails rather than some generic assertion error in utility method.
There was a problem hiding this comment.
To ensure more readable logs I've added also simple implementation of MarkdownRange#toString method.
android/src/test/java/com/expensify/livemarkdown/RangeSplitterTest.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/expensify/livemarkdown/RangeSplitter.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/expensify/livemarkdown/RangeSplitter.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/expensify/livemarkdown/RangeSplitter.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/expensify/livemarkdown/RangeSplitter.java
Outdated
Show resolved
Hide resolved
| public static void splitRangesOnEmojis(List<MarkdownRange> markdownRanges, String type) { | ||
| List<MarkdownRange> emojiRanges = new ArrayList<>(); | ||
| List<MarkdownRange> oldRanges = new ArrayList<>(markdownRanges); | ||
| markdownRanges.clear(); |
There was a problem hiding this comment.
I still don't understand why it's better to modify markdownRanges in-place rather than create a new list. Could you please explain what's the actual benefit from doing so?
There was a problem hiding this comment.
I must admit I might have misunderstood you during our last conversation about this.
I don't really see any benefit. I rewrote the function to not modify ranges in place.
Details
This pr improves time complexity of
splitRangesOnEmojiand adds unit testsRelated Issues
GH_LINK
Manual Tests
The same tests like in linked pr.
Linked PRs
#534