Write additional tests and report errors.

PaulWoitaschek · May 13, 2023 · 6d024c4 · 6d024c4
1 parent 5515aa2
commit 6d024c4
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 30 deletions.
diff --git a/data/src/main/kotlin/voice/data/Chapter.kt b/data/src/main/kotlin/voice/data/Chapter.kt
@@ -11,6 +11,7 @@ import kotlinx.serialization.descriptors.PrimitiveSerialDescriptor
 import kotlinx.serialization.descriptors.SerialDescriptor
 import kotlinx.serialization.encoding.Decoder
 import kotlinx.serialization.encoding.Encoder
+import voice.logging.core.Logger
 import java.time.Instant
 
 @Entity(tableName = "chapters2")
@@ -25,32 +26,47 @@ data class Chapter(
 
   @Ignore
   val chapterMarks: List<ChapterMark> = if (markData.isEmpty()) {
-    listOf(ChapterMark(name, 0L, duration))
+    listOf(ChapterMark(name, 0L, duration - 1))
   } else {
-    val sorted = markData.distinctBy { it.startMs }
-      .filter { it.startMs in 0..<duration }
-      .sorted()
-    val result = mutableListOf<ChapterMark>()
-    for ((index, markData) in sorted.withIndex()) {
-      val name = markData.name
-      val previous = result.lastOrNull()
-      val next = sorted.getOrNull(index + 1)
-      val startMs = if (previous == null) 0L else previous.endMs + 1
-      val endMs = if (next != null && next.startMs + 2 < duration && startMs < next.startMs - 1) {
-        next.startMs - 1
-      } else {
-        duration - 1
-      }
-      result += ChapterMark(
-        name = name,
-        startMs = startMs,
-        endMs = endMs,
-      )
-      if (endMs == duration - 1) {
-        break
+    try {
+      val result = mutableListOf<ChapterMark>()
+      val sorted = markData.distinctBy { it.startMs }
+        .filter { it.startMs in 0..<duration - 2 }
+        .sorted()
+
+      for ((index, markData) in sorted.withIndex()) {
+        val name = markData.name
+        val previous = result.lastOrNull()
+        val next = sorted.getOrNull(index + 1)
+
+        val endMs = if (next != null && next.startMs <= duration - 2) {
+          next.startMs - 1
+        } else {
+          duration - 1
+        }
+
+        if (previous == null) {
+          result += ChapterMark(
+            name = name,
+            startMs = 0L,
+            endMs = endMs,
+          )
+        } else {
+          val startMs = previous.endMs + 1
+          if (startMs < duration && startMs < endMs) {
+            result += ChapterMark(
+              name = name,
+              startMs = startMs,
+              endMs = endMs,
+            )
+          }
+        }
       }
+      result
+    } catch (e: Exception) {
+      Logger.e(e, "Could not parse marks from $this")
+      listOf(ChapterMark(name, 0L, duration - 1))
     }
-    result.ifEmpty { listOf(ChapterMark(name, 0L, duration)) }
   }
 
   override fun compareTo(other: Chapter): Int {

diff --git a/data/src/test/kotlin/voice/data/ChapterTest.kt b/data/src/test/kotlin/voice/data/ChapterTest.kt
@@ -7,12 +7,81 @@ import java.time.Instant
 class ChapterTest {
 
   @Test
-  fun `chapter parsing does best effort on broken marks`() {
+  fun `filters duplicates`() {
+    test(
+      chapterStarts = listOf(0, 5, 5, 10),
+      expected = listOf(
+        MarkPosition(0, 4),
+        MarkPosition(5, 9),
+        MarkPosition(10, 19),
+      ),
+      duration = 20,
+    )
+  }
+
+  @Test
+  fun `missing start position`() {
+    test(
+      chapterStarts = listOf(5, 10),
+      expected = listOf(
+        MarkPosition(0, 9),
+        MarkPosition(10, 19),
+      ),
+      duration = 20,
+    )
+  }
+
+  @Test
+  fun `exceeding end position`() {
+    test(
+      chapterStarts = listOf(0, 10, 19),
+      expected = listOf(
+        MarkPosition(0, 9),
+        MarkPosition(10, 19),
+      ),
+      duration = 20,
+    )
+  }
+
+  @Test
+  fun `marks without duration`() {
+    test(
+      chapterStarts = listOf(0, 5, 6, 7, 10),
+      expected = listOf(
+        MarkPosition(0, 4),
+        MarkPosition(5, 6),
+        MarkPosition(7, 9),
+        MarkPosition(10, 19),
+      ),
+    )
+  }
+
+  @Test
+  fun `on single chapter creates fallback`() {
+    test(
+      chapterStarts = listOf(5),
+      expected = listOf(
+        MarkPosition(0, 19),
+      ),
+    )
+  }
+
+  @Test
+  fun `on missing chapters creates a single fallback`() {
+    test(
+      chapterStarts = listOf(),
+      expected = listOf(
+        MarkPosition(0, 19),
+      ),
+    )
+  }
+
+  private fun test(chapterStarts: List<Int>, expected: List<MarkPosition>, duration: Long = 20L) {
     val positions = Chapter(
-      duration = 20L,
+      duration = duration,
       fileLastModified = Instant.now(),
       id = ChapterId(""),
-      markData = listOf(11, 5, 5, 10).mapIndexed { index, i ->
+      markData = chapterStarts.sorted().mapIndexed { index, i ->
         MarkData(
           startMs = i.toLong(),
           name = "Mark $index",
@@ -21,10 +90,7 @@ class ChapterTest {
       name = "Chapter",
     ).chapterMarks.map { MarkPosition(it.startMs, it.endMs) }
 
-    positions.shouldContainInOrder(
-      MarkPosition(0L, 9L),
-      MarkPosition(10L, 19L),
-    )
+    positions.shouldContainInOrder(expected)
   }
 
   data class MarkPosition(val start: Long, val end: Long)