chore: pre-publish review for v1.8.0 - #71
Merged
Merged
Conversation
…d in own stress test Full review pass before publishing 1.8.0 to pub.dev: docs, temp/dead code sweep, test coverage, demo app run + logs, pana score. ## Doc drift fixed - README + 5 doc/*.md files: install snippet still pinned ^1.7.0 (a version that was never published — folded into 1.8.0 in CHANGELOG, correctly, but the doc pins were missed when that happened). All now ^1.8.0. - example/integration_test/issue_66_69_ftl_test.dart: pre-existing dart format drift from an earlier merged PR, unrelated to this review but trivial and zero-risk to fix now. ## Real bug found and fixed: issue_30 stress false "dropped event" diagnosis While confirming the CHANGELOG's "DartWorker.timeoutMs drops its terminal event" Known-Issues entry (written in the previous session), traced the actual mechanism end to end through BaseKmpWorker.doWorkInternal and NativeWorkmanagerPlugin+EventChannel's observeWorkCompletion fallback. Finding: it was never a dropped event. A timed-out DartWorker returns a *retryable* failure by design (matching #46/#47's "return false retries" fix) — WorkInfo never reaches a terminal state until all default-3 retries with backoff (Android: WorkManager's own; iOS: 30s initial, exponential) are exhausted, which routinely exceeds a short test window. Verified directly: DartWorker(timeoutMs: 1000, delayMs: 2000) with maxRetries: 0 delivers its terminal event at ~1020ms, exactly at the timeout mark, on both platforms — confirmed via disposable diagnostic tests, not assumed. Fixed the real bug this exposed: issue_30 stress in stress_and_system_test.dart never set maxRetries: 0, so its own wait-time budget was racing retry backoff instead of the timeout it meant to test — and its `catch (_) { actuals.add(0) }` fallback folded "no event ever arrived" and "correctly failed" into the same outcome, which is exactly how a real dropped-event regression would have slipped through undetected. Added maxRetries: 0 to the enqueue calls and replaced the silent catch with an explicit expect(neverArrived, isEmpty) that names the case. Full stress_and_system_test.dart now passes end to end (confirmed on iOS simulator) — previously it aborted the whole file after issue_30 stress's TimeoutException, skipping every System Tests case after it. CHANGELOG's "Known Issues" section rewritten to describe what was actually wrong (a test-infrastructure bug) instead of the product bug that was never real. ## Cross-platform test parity gap closed ios/Tests/SecurityValidatorTests.swift had no coverage at all for the sanitizedURL UserInfo-redaction fix shipped in 1.8.0 (Android's parallel SecurityValidatorSanitizedUrlTest.kt did). Added matching cases. Every assertion verified against the real URLComponents/redactUserInfo logic via a standalone Swift script before being written — caught a wrong assumption in the first draft (empty string does NOT return "[invalid URL]"; URLComponents(string: "") is non-nil, matching Android's Uri.parse("") leniency) before it shipped. Documented honestly: ios/Tests/ *.swift is not currently wired into any runnable Xcode test target (no RunnerTests scheme references it) — a pre-existing gap, not something this commit fixes, so this coverage exists for correctness/parity and whenever that target gets wired up, not as something CI runs today. ## Verification - pana: 160/160, both packages - flutter analyze: 0 issues, all 3 packages - Host suite (unit/integration/security/performance): 1815+ tests green - Android Gradle unit tests: 98/98 (incl. new SecurityValidatorSanitizedUrlTest) - native_workmanager_gen: 15/15 - Device (iOS simulator): All Workers (16/16), stress_and_system_test.dart full file including the fixed issue_30 stress and every System Tests case — all green - Demo app: built + launched on iOS simulator, ran a real worker (SyncWorker via Quick Sync) end to end with clean logs, no crashes, 42 BGTask identifiers registered under kmpworkmanager 3.5.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-publish review pass: docs, dead/temp code sweep, test coverage, demo app run.
What's in this PR
^1.7.0(a version never published — its changes folded into 1.8.0's CHANGELOG entry, correctly, but the doc pins were missed). All now^1.8.0.issue_30 stressinstress_and_system_test.dartnever setmaxRetries: 0, so its wait budget raced retry backoff instead of the timeout, and itscatch (_) { actuals.add(0) }masked "no event ever arrived" as indistinguishable from "correctly failed." Fixed both. Verified directly:DartWorker(timeoutMs: 1000, delayMs: 2000)withmaxRetries: 0delivers its terminal event at ~1020ms, exactly at the timeout mark, on both platforms. Fullstress_and_system_test.dartnow passes end-to-end (previously aborted afterissue_30 stress's exception, skipping every System Tests case after it).ios/Tests/SecurityValidatorTests.swifthad no coverage for thesanitizedURLUserInfo-redaction fix shipped in 1.8.0 (Android had it). Added matching cases, each verified against the real logic via a standalone script first — caught a wrong assumption in the draft before it shipped (empty string doesn't return"[invalid URL]", matching Android'sUri.parse("")leniency). Documented honestly thatios/Tests/*.swiftisn't wired into any runnable Xcode test target right now (pre-existing gap, not fixed here).Verification
flutter analyze: 0 issues, all 3 packagesnative_workmanager_gen: 15/15stress_and_system_test.dartincluding the fixedissue_30 stress— all green