Make the dashboard's banner logic a pure, tested function - #315
Merged
Conversation
…unction loadDashboardState was 623 lines, and the ~25 guards that decide which banners a device gets were ~290 of them, each ending in a res.getString. That shape is what kept them untested: every branch needed a real Resources, this module has no Robolectric, and so the single most user-visible piece of logic in the app had zero coverage while the pure detectors feeding it had plenty. Split along the seam the codebase already uses elsewhere — classifyKmodProblem /renderKmodProblem, classifyKpmProblem/renderKpmProblem. dashboardIssues() takes a DashboardFacts and returns List<DashboardIssue>, no Android types anywhere; DashboardIssue.toMessage() words one. A case carries the data its wording needs rather than a preformatted string, so the split is real and not cosmetic. filesystemHidingDashboardMessage was the odd one out doing both halves itself; it folds in, and its file goes. Emission order is load-bearing — the screen groups by severity but keeps emission order within a group, so this decides which error a user reads first. It used to be implied by the physical layout of one long block, where inserting a guard in the wrong place silently reordered banners. It is now eight named calls in one place, pinned by a test. The four backends' diagnosis blocks were near-identical eighteen-line copies differing only in kind, activator path and classifier; they collapse into deriveModuleFact. Derivation splits into deriveModuleFacts / deriveLsposedFacts / deriveEnvironmentFacts / resolveProtectionFacts, leaving an orchestrator that reads as the four steps it always was. Both detekt suppressions come off — the gate now passes on merit rather than by exemption. Behaviour is unchanged, checked emission by emission against the original: all 34 sites map one-to-one, in the same order, with the same severities. Two things that only looked like changes: the non-kmod module problems dropped their downloadArtifact at the callsite, which was a no-op because only renderKmodProblem ever sets one, and Active.version being null already meant no mismatch banner because versionsMismatch returns false for it. One dead local (zygiskStatusRaw) went with the rewrite. Also fixes a mark the profiler has been asking for and never received: measure-startup.py reads dashboard_issues_done, the code only ever emitted dashboard_messages_done, so the "issues" stage has always measured nothing.
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.
loadDashboardStatewas 623 lines, ~290 of which were the guards deciding which banners a device gets. Each ended in ares.getString, which is what kept them untested — every branch needed a realResourcesand this module has no Robolectric, so the most user-visible logic in the app had no coverage while the pure detectors feeding it had plenty.The split follows the seam already used elsewhere in the codebase (
classifyKmodProblem/renderKmodProblem):dashboardIssues()takes aDashboardFactsand returnsList<DashboardIssue>with no Android types anywhere, andDashboardIssue.toMessage()words one. Both detekt suppressions come offloadDashboardState— it passes the gate on merit now, at 55 lines.Behaviour is unchanged, verified emission by emission against the original: all 34 sites map one-to-one, in the same order, with the same severities. Two things that only looked like changes are called out in the commit body.
DashboardIssue(28 cases, grouped into six sub-interfaces) and the puredashboardIssues(DashboardFacts)DashboardIssueRender.kt— the only half that needs aContextfilesystemHidingDashboardMessageinto the same two halves and delete its filederiveModuleFactderiveModuleFacts/deriveLsposedFacts/deriveEnvironmentFacts/resolveProtectionFactsDashboardIssuesTest— 31 tests, including one pinning banner orderdashboard_issues_done, the markmeasure-startup.pyhas always read and never received, and correct the stage spans around itAGENTS.md