test/refactor(core): add unit tests for logcat data source related logic

[Manabu-GT]

Summary by CodeRabbit

• Refactor

  • Extracted log parsing into a dedicated parser and introduced a thread-name cache for cleaner, modular parsing.

• Tests

  • Added comprehensive unit tests covering parsing, thread-name resolution, malformed lines, and edge cases.

• Documentation

  • Expanded testing guidelines with advice on organizing large test files and using code regions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[claude]

Claude finished @Manabu-GT's task in 3m 22s —— View job

---

PR Review: Logcat Data Source Refactoring & Tests

This is a well-executed refactoring that improves testability and separation of concerns. The extraction of LogcatEntryParser and ThreadNameCache into dedicated classes is clean and idiomatic. Overall code quality is excellent with good test coverage.

---

Summary

Strengths:

• ✅ Excellent separation of concerns - parsing and caching logic properly extracted
• ✅ Idiomatic Kotlin patterns throughout (null-safe operators, extension functions, coroutines)
• ✅ Proper thread safety with @GuardedBy annotations and synchronized blocks
• ✅ Comprehensive test coverage with good edge case handling
• ✅ Correct coroutine usage with appropriate dispatchers (Dispatchers.IO for blocking operations)
• ✅ Good error handling for IOException and SecurityException
• ✅ Updated TESTING.md documentation

Issues Found:

• 🐛 1 bug: Regex won't match empty log messages (see inline comment on LogcatEntryParser.kt:10)
• ⚠️ Minor improvements: Thread safety documentation and test coverage gaps (see inline comments)

---

Detailed Findings

1. Bug: Empty Messages Not Parsed (Priority: Medium)

• File: LogcatEntryParser.kt:10
• Issue: Regex pattern (.+) requires at least one character in message, so empty messages won't parse
• Fix: Change (.+) to (.*) in the regex pattern
• See inline comment →

2. Thread Safety: I/O in Synchronized Block (Priority: Low)

• File: ThreadNameCache.kt:32
• Issue: File I/O happens while holding lock, which could cause contention under high throughput
• Current verdict: Acceptable for single-threaded logcat parsing, but worth documenting
• See inline comment →

3. Test Coverage Gaps (Priority: Low)

• ThreadNameCacheTest: Missing tests for tid <= 0 edge cases and LRU eviction behavior
• LogcatEntryParserTest: Missing test for empty messages (will be needed after fixing the regex bug)
• See inline comments for suggested tests →

4. Minor: Flow Emission Semantics (Priority: Low)

• File: LogcatDataSource.kt:70
• Suggestion: Consider using addAndSnapshot() for clearer immutability guarantees
• See inline comment →

---

Architecture Review

The refactoring successfully achieves:

1. Testability: Parser and cache logic can now be tested independently without Android runtime
2. Single Responsibility: Each class has a focused purpose
3. Dependency Injection: Parser accepts ThreadNameCache as a dependency, enabling test doubles
4. Maintainability: Simpler classes are easier to understand and modify

The original LogcatDataSource went from ~150 lines to ~140 lines while gaining two well-tested helper classes.

---

Test Quality Assessment

Excellent aspects:

• Using Google Truth assertions consistently
• Descriptive test names with backticks
• Good coverage of valid inputs, whitespace handling, special characters, and invalid inputs
• Proper use of test doubles (injecting threadNameResolver lambdas)
• Following project's testing guidelines from TESTING.md

Suggested additions:

• Edge case tests mentioned in inline comments
• Consider adding a test that verifies timestamp parsing edge cases (year 2038+ timestamps)

---

Recommendation

Approve with minor fixes. The only blocking issue is the regex bug for empty messages. The other suggestions are optional improvements.

Action items:

1. ✅ Must fix: Update regex pattern to handle empty messages (LogcatEntryParser.kt:10)
2. 🔄 Should add: Test for empty message parsing
3. 💡 Optional: Add tests for edge cases mentioned in inline comments
4. 💡 Optional: Document thread safety assumptions in ThreadNameCache.resolve()

---

[coderabbitai]

Walkthrough

Logcat line parsing was extracted into a new LogcatEntryParser and a ThreadNameCache. LogcatDataSource now accepts a parser dependency and delegates line parsing to it; ThreadNameCache resolves/caches thread names (reading /proc when needed). Unit tests and a docs update were added.

Changes

Cohort / File(s)	Summary
Core refactor debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt	Replaced inline logcat line parsing with injected LogcatEntryParser; removed regex, thread-name cache, and helper methods; updated constructor to accept parser and maxEntries.
New parser component debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatEntryParser.kt	New LogcatEntryParser class: regex-based epoch-format parsing, epoch→ms conversion, numeric pid/tid parsing, LogLevel mapping, and thread-name lookup via injected ThreadNameCache.
New cache component debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/ThreadNameCache.kt	New ThreadNameCache with LRU cache, @Synchronized fun resolve(pid, tid): String, special-case "main" when pid==tid, proc-fs resolver fallback, and cache get-or-put helper.
Parser tests debugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatEntryParserTest.kt	Tests for valid parsing, timestamp conversion, pid/tid extraction, thread-name resolution, whitespace and special-character handling, empty messages, and malformed-line rejection.
Cache tests debugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/data/source/ThreadNameCacheTest.kt	Tests for resolve behavior: main-thread detection, resolver delegation, fallback for invalid tids/resolver nulls, and caching semantics.
Log level tests debugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/model/LogLevelTest.kt	Tests for LogLevel.fromString() and LogLevel.fromInt() mappings and defaults.
Documentation docs/TESTING.md	Added "Code Regions in Tests" guidance for using region/endregion markers in tests.

Sequence Diagram(s)

sequenceDiagram
  participant LD as LogcatDataSource
  participant P as LogcatEntryParser
  participant C as ThreadNameCache
  participant R as /proc resolver

  LD->>P: parse(line)
  P->>C: resolve(pid, tid)
  C->>R: read /proc/self/task/{tid}/comm (if not cached & pid!=tid)
  R-->>C: threadName? (or null)
  C-->>P: threadName (cached or fallback)
  P-->>LD: LogEntry? (parsed or null)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I sniffed the lines at break of dawn,
Parsed epochs, tags, and threads anon,
Cached names in burrows snug and deep,
Tests hop by hop to guard the keep,
A tiny patch—now watch it run! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.04% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: introducing unit tests for logcat data source parsing logic while refactoring the codebase to extract parsing into dedicated classes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @Manabu-GT's task in 1m 3s —— View job

---

PR Re-Review: Logcat Data Source Tests

Tasks

• Read all changed files
• Check if regex bug was fixed
• Verify test coverage additions
• Review author's feedback on previous suggestions
• Check for any new issues
• Post comprehensive review

---

Summary

The author has addressed the critical issues from the previous review. This PR is now in excellent shape and ready to merge.

What was fixed:

• ✅ Regex bug fixed: Changed (.+) to (.*) to handle empty messages (line 10)
• ✅ Added empty message test: New test verifies empty messages parse correctly
• ✅ Added invalid tid tests: New test covers tid <= 0 edge cases
• ✅ Improved main thread test: Now properly verifies the resolver isn't called for pid==tid

Author's responses to previous suggestions:

• ✅ addAndSnapshot() suggestion: Author correctly noted that .throttleLatest(500ms) makes this optimization invalid—the throttling specifically minimizes list creation, so the current implementation is correct
• ✅ Thread safety concern: Author appropriately assessed that the contention is minimal for the single-threaded logcat parsing use case and can be optimized later if needed

---

Final Assessment

Code Quality: Excellent

• Clean separation of concerns with LogcatEntryParser and ThreadNameCache
• Idiomatic Kotlin with proper null safety and coroutine usage
• Thread safety properly implemented with @GuardedBy and synchronized blocks
• Appropriate error handling for IOException and SecurityException

Test Coverage: Comprehensive

• Valid input parsing with all field extraction
• Edge cases: empty messages, whitespace variations, special characters
• Invalid input rejection: malformed lines, missing fields, non-numeric values
• Thread name resolution: main thread detection, caching, fallback behavior
• Invalid tid handling: zero and negative values

Architecture: Well-designed

• Testability improved by extracting parsing logic
• Dependency injection enables test doubles
• Single responsibility principle maintained
• Original ~150-line class now properly decomposed

---

Recommendation

✅ Approve and merge. All critical issues have been resolved, test coverage is thorough, and the code quality is excellent. The refactoring successfully achieves its goals of improving testability and maintainability.

Optional future improvements (not blocking):

• Consider adding LRU eviction behavior tests if thread count grows significantly
• Could add timestamp parsing edge case tests (e.g., year 2038+ timestamps)
• Document thread safety assumptions in ThreadNameCache.resolve() if multi-threaded access becomes a concern

---

Great work on this refactoring! The extraction of parsing logic into dedicated, well-tested classes is a solid improvement to the codebase.

---