summaryrefslogtreecommitdiff
path: root/app/src/test/java/org/terst/feedreader/readability
diff options
context:
space:
mode:
authorPeter Stone <thepeterstone@gmail.com>2026-09-20 17:39:04 +0000
committerPeter Stone <thepeterstone@gmail.com>2026-09-20 17:39:04 +0000
commitcf8ff865f7764953ded1b93c8ec42d4aff589e81 (patch)
tree186ef308bfd2cfa46504052550cb8ace7a44d2ae /app/src/test/java/org/terst/feedreader/readability
parentf1d03f7bd675050a088e7baf3de648088e7f7e4b (diff)
Fix intermittent extraction failures, and stop them being silent
Bug report: extraction failed on one 1500days.com article while others worked. It is not that article. Extraction was racing the page load, and the report could say nothing more because the app discarded the reason. Two separate defects. 1. The race. onPageFinished fires before the page has finished loading — observed at progress=25 on this article — and the extraction script was injected there. Sometimes the DOM is far enough along; sometimes evaluateJavascript hands back a bare "null" because the document is still being replaced. Measured against the reported URL: 4 successes in 6 attempts before, 10 in 10 after. An unusable result is no longer final. A null or empty-content result is retried (6 attempts, 500ms apart, well inside the existing 25s timeout); a Readability error still fails immediately, since retrying will not change what the parser thinks of the markup. 2. The silence. ReaderScreen did `extracted?.content?.let { }`, so every failure — dead host, HTTP error, timeout, parser throw — arrived as the same null and was dropped. The reader just kept showing raw feed content. The only way a failure ever surfaced was the user noticing. extractResult() now returns a typed ExtractionFailure carrying the reason; ReaderScreen logs the diagnostic and shows the user a short explanation. Also replaced a dead error callback. The only override was the four-argument onReceivedError, which Android stopped calling at API 23; minSdk is 26, so it never fired on any supported device. A failed load therefore invoked nothing and hung for the full 25 seconds before surfacing as an indistinguishable null. Now overrides the request/error and onReceivedHttpError forms Android actually calls, both filtered to the main frame so a 404'd tracker cannot abort an article that renders fine. The instrumented test does NOT reproduce the race and says so in its own docstring: a MockWebServer page finishes loading before onPageFinished fires, and slow subresources do not help because WebView waits for them. It is kept as a reliability guard over repeated back-to-back extractions. The race was verified by hand against the live URL. 47 unit tests, 1 instrumented test, both green.
Diffstat (limited to 'app/src/test/java/org/terst/feedreader/readability')
-rw-r--r--app/src/test/java/org/terst/feedreader/readability/ExtractionFailureTest.kt82
1 files changed, 82 insertions, 0 deletions
diff --git a/app/src/test/java/org/terst/feedreader/readability/ExtractionFailureTest.kt b/app/src/test/java/org/terst/feedreader/readability/ExtractionFailureTest.kt
new file mode 100644
index 0000000..6f691fd
--- /dev/null
+++ b/app/src/test/java/org/terst/feedreader/readability/ExtractionFailureTest.kt
@@ -0,0 +1,82 @@
+package org.terst.feedreader.readability
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * Extraction failures used to be discarded entirely — ReaderScreen did
+ * `extracted?.content?.let { }`, so any failure left the reader silently showing
+ * the raw feed content. The bug report that prompted this could say only
+ * "extraction failed", because the app itself never knew why.
+ *
+ * These pin the two things a failure has to produce: something short enough to
+ * put in front of the user, and something specific enough to debug from.
+ */
+class ExtractionFailureTest {
+
+ @Test
+ fun `load error names the underlying description`() {
+ val f = ExtractionFailure.LoadError(-2, "net::ERR_NAME_NOT_RESOLVED")
+ assertEquals("Couldn't load the page (net::ERR_NAME_NOT_RESOLVED)", f.userMessage())
+ assertTrue(f.diagnostic().contains("code=-2"))
+ assertTrue(f.diagnostic().contains("ERR_NAME_NOT_RESOLVED"))
+ }
+
+ @Test
+ fun `load error without a description still reads properly`() {
+ assertEquals("Couldn't load the page", ExtractionFailure.LoadError(-1, null).userMessage())
+ assertEquals("Couldn't load the page", ExtractionFailure.LoadError(-1, " ").userMessage())
+ }
+
+ @Test
+ fun `http error names the status`() {
+ val f = ExtractionFailure.HttpError(403, "Forbidden")
+ assertEquals("Server returned HTTP 403", f.userMessage())
+ assertTrue(f.diagnostic().contains("status=403"))
+ }
+
+ @Test
+ fun `timeout is reported in seconds`() {
+ assertEquals("Page took longer than 25s to load", ExtractionFailure.TimedOut(25_000).userMessage())
+ assertTrue(ExtractionFailure.TimedOut(25_000).diagnostic().contains("25000ms"))
+ }
+
+ @Test
+ fun `readability error carries the thrown message`() {
+ val f = ExtractionFailure.ReadabilityError("TypeError: x is null")
+ assertTrue(f.userMessage().contains("TypeError"))
+ assertTrue(f.diagnostic().contains("TypeError"))
+ }
+
+ @Test
+ fun `no content is distinct from an error`() {
+ assertEquals("No article text found on the page", ExtractionFailure.NoContent.userMessage())
+ }
+
+ // The raw JS result can be enormous; a diagnostic that pastes all of it is
+ // unusable in a bug report.
+ @Test
+ fun `bad result truncates the raw payload`() {
+ val f = ExtractionFailure.BadResult("x".repeat(5_000))
+ assertTrue("diagnostic should be bounded", f.diagnostic().length < 400)
+ }
+
+ // Every case must produce a non-empty message; a blank one would put the
+ // user back where they started.
+ @Test
+ fun `every failure produces both messages`() {
+ val all = listOf(
+ ExtractionFailure.LoadError(-6, "ERR_CONNECTION_REFUSED"),
+ ExtractionFailure.HttpError(500, null),
+ ExtractionFailure.TimedOut(25_000),
+ ExtractionFailure.ReadabilityError("boom"),
+ ExtractionFailure.NoContent,
+ ExtractionFailure.BadResult("{}"),
+ )
+ for (f in all) {
+ assertTrue("$f has a blank user message", f.userMessage().isNotBlank())
+ assertTrue("$f has a blank diagnostic", f.diagnostic().isNotBlank())
+ }
+ }
+}