Repository navigation
test(tooling): stop pending Hive opens from hanging later tests - #9057
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
CI is red on Run: https://github.com/divinevideo/divine-mobile/actions/runs/34542608847/job/103088446784 This reads as a regression from this PR's test-tooling change rather than the #9022 hang it targets: the failures are concentrated in one suite and repeat, not a 10-minute timeout in |
This comment has been minimized.
This comment has been minimized.
rabble
left a comment
There was a problem hiding this comment.
Requesting changes: one real regression in the shared test seam, plus a guard that now passes vacuously.
Blocking
mobile/test/services/hive_box_opener_test.dart:37 — tearDown sets HiveBoxOpener.observerForTesting = null. Under very_good --optimization, flutter_test_config.testExecutable runs once for the whole bundle and installs the shared observer at bundle start, so nulling it here permanently disables the real-zone open seam for every suite that runs after this file. That reintroduces the #9022 hang and removes the pending-open attribution strict mode depends on. Capture the prior observer in setUp and restore it in tearDown, or drop the null.
Non-blocking
mobile/test/constants/hive_box_names_test.dart:40 — after moving the app call sites to HiveBoxOpener.open(...), the _openBoxCall pattern (.open(?:Lazy)?Box() no longer matches any app call site, so the #6919 "every box named via HiveBoxNames" guard scans nothing and passes vacuously. The comment "Its callers remain covered here" is no longer true. Extend the scanner to match HiveBoxOpener.open/openLazy, or scan the opener's callers.
Questions
- The PR verifies focused suites (35 tests). Was the full
--optimizationbundle run underDIVINE_STRICT_HIVE_BOXES=truebefore enabling it in CI? If unrelated pre-existing leaks remain, strict mode could turn CI red. mobile/test/providers/social_providers_cleanup_test.dart:120adds cleanup for the notifications box, but this suite never references it. Is the leak owned here, or is this masking a leak strict mode should attribute elsewhere?
|
Author update: the current head
Verification on the current head:
|
This comment has been minimized.
This comment has been minimized.
|
Addressed every item from the latest review:
Current head |
|
Overlap heads-up: this PR and draft PR #8540 both change repository_providers.dart and repository_providers.g.dart. The handwritten changes are in different provider sections, but the generated hash will conflict. Please coordinate merge order; whichever lands second should rebase onto fresh origin/main, regenerate the provider output, and rerun the focused Hive-opening and people-list provider tests. |
dcadenas
left a comment
There was a problem hiding this comment.
@NotThatKindOfDrLiz the head at 57978d8d340e addresses rabble's earlier review: the root Hive observer is captured and restored, and the box-name scanner is effective again for app-owned Hive opens. I reviewed that revision and have nothing blocking it.
Checks
- Mobile CI run 34672113021 on this exact commit: success. Analyze, Format, Generated Files, Goldens, all four Tests shards, VGV Tag Gate, CI Configuration Tests, Codemagic Variable Groups, Codex Configuration, and Mobile CI pass.
- Shard 0's job log records
--dart-define=DIVINE_STRICT_HIVE_BOXES=truein the test flags, and all four shards pass. - Build Flutter Web Preview, Integration Tests (services), mergeability, semantic title, and CLA checks pass.
- Locally:
dart formatandflutter analyzepass, 49 focused Hive tests pass, and a wider run of 10 Hive-touching suites under both strict flags passes (133 tests).scripts/check_direct_hive_opens.shpasses with an empty baseline. - A non-vacuity probe confirmed the scanner fails and names an offender when an app-owned open uses an unknown box name; the probe was then removed.
- The full local
flutter testrun hit the per-command time limit under machine load and did not complete. The CI run above is the merged-bundle evidence for this commit.
This approval follows an independent review of the same revision.
@rabble your change request predates 57978d8d340e and your re-review request is already pending, so nothing was re-requested here. Take another look when you can.
Merge stays with you; your #8540 merge-order note does not change this review.
|
Additional overlap heads-up: #9120 now intersects this PR in upload_initialization_helper.dart and social provider cleanup coverage. Please coordinate merge order; whichever lands later should rebase onto fresh origin/main and verify pending Hive opens remain isolated after the upload package extraction. |
57978d8 to
3dd0603
Compare
Mobile PR PreviewPreview refreshed for Last refresh:
|
|
Author update: #9057 is rebased onto current
Verification on the exact current head:
@rabble @dcadenas please re-review this rebased head. The remaining |
rabble
left a comment
There was a problem hiding this comment.
Approved. No remaining findings on this revision.
Reviewed commit 30ca5f8fea98b00ef09c8ca1a3ac1a62352269d3.
The root Hive observer is now captured and restored, preserving the observer for subsequent suites in the optimized test isolate. The box-name scanner matches both HiveBoxOpener.open and openLazy, so production call sites remain covered. Notification-box cleanup belongs to the social-provider cleanup suite because its UserDataCleanupService uses the Hive-backed preferences store. The strict flags are enabled in CI.
Independently ran the opener, shared Hive guard, box-name scanner, and social-provider cleanup suites: 43 tests passed in ordinary mode and 43 passed in optimized merged mode, both with strict channel and Hive guards. Focused analysis and the direct-Hive-open guard passed. All four exact-head CI shards are green. No new findings. Coordinate the Hive seam with #9120's upload extraction.
Problem
Flutter widget tests can start a real Hive box open inside fake async and leave Hive’s private opening registry waiting forever. The existing root guard only sees boxes after they are fully open, so a later suite opening the same name can spend ten minutes waiting on an unrelated test. The current
/hashtag/rustroute test is one concrete trigger.Why this fix
App-owned box opens now pass through a thin
HiveBoxOpenerseam. Production still delegates directly to Hive. The merged test harness installs an observer that starts the Hive operation in its captured real-async zone, records it until settlement, and gives root teardown enough visibility to await, heal, and attribute pending work to the test that started it.This deliberately avoids
HiveImpl.unregisterBox(): removing a private registry entry would not cancel the original I/O and could allow two opens to race. A zero-baseline generated-files guard prevents production code from bypassing the observable seam.Strict Hive attribution is enabled in Mobile CI and
mise run test. The existing hashtag route test also uses the same mocked service pattern as the focused hashtag screen tests, so it no longer starts storage that its routing assertion does not exercise.What this leaves alone
Verification
flutter analyze lib test integration_testCloses #9053
Closes #9022
Supersedes #9050 by including its approved route-test fix.