Skip to content

fix: improve core:network module quality and test coverage - #1493

Open
lihenggui wants to merge 7 commits into
mainfrom
fix/network-module-improvements
Open

fix: improve core:network module quality and test coverage#1493
lihenggui wants to merge 7 commits into
mainfrom
fix/network-module-improvements

Conversation

@lihenggui

Copy link
Copy Markdown
Owner

Summary

Comprehensive fixes for the core:network module addressing bugs, design issues, and test coverage gaps.

Bug Fixes

  • Fix integer division in progress calculationBinaryFileWriter reported 0% progress until the very end due to Long / Long integer division. Changed to totalBytes.toDouble() / length * 100.0
  • Fix response body leakdownloadRules() never closed the OkHttp response body, causing connection pool leaks. Now uses response.use { ... }
  • Fix blocking I/O on caller threaddownloadRules() used synchronous .execute() instead of the async .await() coroutine bridge, blocking the caller's thread
  • Fix DI-provided Json not being usedRetrofitBlockerNetwork used the global Json default instead of the Hilt-provided instance configured with ignoreUnknownKeys = true

Design Improvements

  • Proper error propagation — replaced silent error swallowing (returning empty strings/0) with NetworkException, allowing callers (e.g., SyncWorker) to catch, retry, or surface errors
  • Remove unused Retrofit dependency — the module used only OkHttp directly; Retrofit was a dead dependency
  • Rename RetrofitBlockerNetworkOkHttpBlockerNetwork — name now matches the actual implementation
  • Remove unused FakeAssetManager from production DI — it was registered in SingletonComponent but never injected anywhere
  • Increase buffer sizeBinaryFileWriter chunk size from 1KB to 8KB for better throughput
  • Update SyncWorker — consolidated error handling to properly catch network exceptions from the new throwing API

Test Coverage

  • BinaryFileWriterUnitTest — 7 tests covering: basic write, empty input, multi-chunk large files, progress reporting accuracy, integer division regression, negative length edge case
  • OkHttpBlockerNetworkTest — 10 tests covering: GitHub/GitLab JSON parsing, empty/blank/invalid JSON, missing fields, extra fields, multiple commits, blank SHA
  • NetworkModelMapperTest — 6 tests covering: full field mapping, null preservation, round-trip conversion for NetworkComponentDetail and NetworkGeneralRule

Test plan

  • ./gradlew :core:network:testFossDebugUnitTest — all 23 tests pass
  • ./gradlew assembleFossDebug — full build succeeds
  • ./gradlew :sync:work:compileFossDebugKotlin — sync module compiles with SyncWorker changes
  • ./gradlew :core:network:spotlessCheck — code formatting verified
  • ./gradlew :app-compose:dependencyGuardBaseline — baseline updated (Retrofit removed)

- Fix integer division bug in BinaryFileWriter progress calculation
  (totalBytes.toDouble() / length instead of totalBytes / length)
- Fix response body leak in downloadRules (use response.use{})
- Fix downloadRules using blocking .execute() instead of async .await()
- Inject DI-provided Json instance instead of using global default
- Replace silent error swallowing with NetworkException for proper
  error propagation to callers
- Remove unused Retrofit dependencies (only OkHttp is used)
- Rename RetrofitBlockerNetwork to OkHttpBlockerNetwork
- Remove unused FakeAssetManager from production DI graph
- Increase BinaryFileWriter chunk size from 1KB to 8KB
- Update SyncWorker to handle network exceptions properly
- Add comprehensive tests for JSON parsing, progress tracking,
  model mappers, and edge cases
- Update dependency guard baseline
NetworkException belongs in the top-level network package, not the
retrofit subpackage, since Retrofit has been removed.
Move ContinuationCallback, OkHttpBlockerNetwork, and its test from
the retrofit subpackage to okhttp, completing the Retrofit removal.
Use the project's @dispatcher(IO) qualifier to inject the IO
dispatcher, consistent with the rest of the codebase and testable
with UnconfinedTestDispatcher.
Change-Id: Icaeaa6549d2f53c35f3da6a4f7bbf48f8ef69007
…provements

# Conflicts:
#	app-compose/dependencies/marketReleaseRuntimeClasspath.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant