From 2b7054b70c33f3b54ed9f795f20318d53240bcad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 05:20:19 +0300 Subject: [PATCH 1/8] feat(perf): assemble the performance data path, in the one package that can see all four dusk reports numbers produced by telescope, wind and magic, and its frozen dependency contract forbids it from importing any of them. This package is the only place in the ecosystem where all four are visible at once, so this is where the seam gets closed: four settable pointers dusk declares with no-op defaults, assigned here to the real sources. The failure this class exists to prevent is silent. Every pointer's default is structurally complete rather than null, which is what lets both sides compile independently, and also what means an unassigned one produces a report of zeros instead of an error, in a different repository, at the end of a driven run that looked like it worked. So the tests seed the store, install, and read back THROUGH each pointer rather than asserting that install() did not throw. Ordering is load-bearing in two places. The observer registers before the idempotency guard is armed, because MagicRouter.addObserver throws once the router has been built and marking the integration installed first would turn a legitimate retry into a silent no-op. And the whole install belongs in installPre, ahead of Magic.init, for the same reason: the throw is deliberately not caught, since a swallowed one leaves the report with no route transitions and nothing to explain their absence. The session hooks are scoped rather than global. Begin zeroes wind's counters, turns counting on, clears telescope's frame buffer and clears the magic-side counters; end turns counting back off. Without the magic-side clear the magic section would report the sum of every previous session while the wind and frame sections reported only the current one, which is the kind of number that is worse than no number. clearFramePerf() rather than clear(), so the HTTP, log and exception buffers a developer may be reading alongside the session survive it. --- CHANGELOG.md | 19 +++ lib/src/magic_devtools.dart | 11 ++ lib/src/perf_integration.dart | 208 +++++++++++++++++++++++ test/perf_integration_test.dart | 292 ++++++++++++++++++++++++++++++++ 4 files changed, 530 insertions(+) create mode 100644 lib/src/perf_integration.dart create mode 100644 test/perf_integration_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index def78e7..412c497 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `MagicPerfIntegration`: the wiring that assembles the performance-diagnostic + data path across four packages. It sets `MagicController.onRefreshUI` to a + counter keyed by controller runtime type, registers a `NavigatorObserver` + through `MagicRouter.addObserver` that times each route push to the first + post-frame callback after the new route builds, calls + `Wind.installPerfResolver()`, registers telescope's `FramePerfWatcher`, and + assigns the four `fluttersdk_dusk` pointers (`framePerfReader`, + `perfExtrasReader`, `perfSessionBeginHook`, `perfSessionEndHook`). This + package is the only place dusk, telescope, wind and magic are all visible at + once, so it is the only place those pointers can be assigned; dusk declares + them with no-op defaults and never imports the packages it reports on. +- `MagicDevtools.installPre()` now installs `MagicPerfIntegration`. It belongs + in the pre-`Magic.init()` half because `MagicRouter.addObserver` throws once + the router has been built, and that `StateError` is deliberately not caught: + a silently unregistered observer would produce a report with no route + transitions and nothing to explain their absence. + ## [0.0.3] - 2026-08-05 Documentation only; no runtime change. The package code is identical to 0.0.2. diff --git a/lib/src/magic_devtools.dart b/lib/src/magic_devtools.dart index 4c8958b..d4b687c 100644 --- a/lib/src/magic_devtools.dart +++ b/lib/src/magic_devtools.dart @@ -2,8 +2,11 @@ import 'package:fluttersdk_dusk/dusk.dart'; import 'package:fluttersdk_telescope/telescope.dart'; import 'dusk_integration.dart'; +import 'perf_integration.dart'; import 'telescope_integration.dart'; +export 'perf_integration.dart'; + /// One-call wiring for the Magic dev-tooling bundle: fluttersdk_dusk + /// fluttersdk_telescope and their Magic integrations, installed in the two /// phases that straddle [Magic.init]. @@ -57,6 +60,12 @@ class MagicDevtools { /// [DumpWatcher]: the two watchers telescope leaves opt-in but every Magic /// dev session wants (uncaught exceptions and `debugPrint` dumps). /// + /// Also installs [MagicPerfIntegration], the performance data path. It + /// belongs in this half rather than [installPost] because it registers a + /// [NavigatorObserver] through `MagicRouter.addObserver`, which throws once + /// the router has been built; the rest of its wiring would work from either + /// half and is kept with it at the one install site. + /// /// Each underlying install is idempotent, so a second call in the same /// isolate is safe. Register additional watchers after this call via /// [TelescopePlugin.registerWatcher]. @@ -66,6 +75,8 @@ class MagicDevtools { TelescopePlugin.install(); TelescopePlugin.registerWatcher(ExceptionWatcher()); TelescopePlugin.registerWatcher(DumpWatcher()); + + MagicPerfIntegration.install(); } /// Post-`Magic.init()` half: wire Magic's runtime into both tools. diff --git a/lib/src/perf_integration.dart b/lib/src/perf_integration.dart new file mode 100644 index 0000000..22eb7b4 --- /dev/null +++ b/lib/src/perf_integration.dart @@ -0,0 +1,208 @@ +import 'dart:collection'; + +import 'package:flutter/scheduler.dart'; +import 'package:flutter/widgets.dart'; +import 'package:fluttersdk_dusk/dusk.dart' + show + framePerfReader, + perfExtrasReader, + perfSessionBeginHook, + perfSessionEndHook; +import 'package:fluttersdk_telescope/telescope.dart'; +import 'package:magic/magic.dart'; + +/// Assembles the whole performance-diagnostic data path: magic's controller and +/// route activity, wind's aggregate counters, telescope's frame buffer, and the +/// four pointers `fluttersdk_dusk` reads them all through. +/// +/// Host integration (debug-only, and BEFORE `Magic.init()`; see [install]): +/// ```dart +/// if (kDebugMode) MagicDevtools.installPre(); +/// ``` +/// +/// This package is the only place in the ecosystem where dusk, telescope, wind +/// and magic are all visible at once, which is why the pointer assignment can +/// only live here: dusk's frozen dependency contract forbids it from importing +/// any of the three packages whose data it reports. +/// +/// The failure mode this class exists to prevent is silent. Every pointer has a +/// structurally-complete no-op default, so an unassigned one produces a report +/// of zeros rather than an error, in a different repository, at the end of a +/// driven run that looked like it worked. +/// +/// `fluttersdk_wind` is reached through magic's barrel, which re-exports it +/// wholesale (`magic/lib/magic.dart:4`); importing it directly here would be +/// flagged as an unnecessary import. +class MagicPerfIntegration { + MagicPerfIntegration._(); + + /// How many route transitions are retained. A long session navigates far + /// more than a report can rank, and the recent ones are the ones near the + /// interaction the operator just drove. + static const int _maxRouteTransitions = 200; + + /// Idempotent install. Safe to call multiple times within the same isolate + /// lifetime. + /// + /// MUST run before the router is built, i.e. from + /// `MagicDevtools.installPre()` ahead of `Magic.init()`: + /// [MagicRouter.addObserver] throws a [StateError] once `routerConfig` has + /// been read (`magic/lib/src/routing/magic_router.dart:158`). That throw is + /// deliberately not caught. A swallowed one would leave the report with no + /// route transitions and nothing to explain their absence. + static void install() { + if (_installed) return; + + // 1. The only step that can fail, so it runs before the idempotency guard + // is armed: marking the integration installed and then throwing would + // turn a retry into a silent no-op. + MagicRouter.instance.addObserver(_observer); + _installed = true; + + // 2. magic: one hook on the single notifyListeners() call site in + // MagicController, counted per controller runtime type so the report can + // name which controller is rebuilding the screen. + MagicController.onRefreshUI = _recordNotify; + + // 3. wind and telescope: the two producers. Installing wind's resolver + // costs nothing on its own; counting stays off until a session's begin + // hook enables it. + Wind.installPerfResolver(); + final FramePerfWatcher watcher = FramePerfWatcher(); + TelescopePlugin.registerWatcher(watcher); + _watcher = watcher; + + // 4. The four dusk pointers. Each returns exactly the key set pinned in + // `dusk/lib/src/utils/perf_readers.dart`; the consumer is in another + // repository, so a renamed key is invisible until a driven run. + framePerfReader = () => { + 'frames': TelescopeStore.recentFramePerf() + .map>((FramePerfRecord r) => r.toJson()) + .toList(), + 'livenessCounter': FramePerfWatcher.livenessCounter, + }; + perfExtrasReader = () => { + 'controllerNotifies': controllerNotifyCounts, + 'routeTransitions': routeTransitions, + }; + perfSessionBeginHook = () { + WindPerfCounters.reset(); + WindPerfCounters.enabled = true; + // clearFramePerf(), never clear(): the latter wipes the HTTP, log and + // exception buffers a developer may be reading alongside the session, + // and resetForTesting() is @visibleForTesting and would fail analysis. + TelescopeStore.clearFramePerf(); + // The magic-side counters are session-scoped for the same reason wind's + // are: without this, every session reports the sum of all previous ones. + _controllerNotifies.clear(); + _routeTransitions.clear(); + }; + perfSessionEndHook = () { + // Counting off, totals intact: `perf_end` reads them to build its + // report, and `WindParser.parse` is too hot to leave instrumented. + WindPerfCounters.enabled = false; + }; + } + + /// Whether [install] has been called at least once. + @visibleForTesting + static bool get isInstalled => _installed; + + /// How many times each controller type has called `refreshUI()` since the + /// last session began, keyed by `runtimeType.toString()`. + static Map get controllerNotifyCounts => + Map.of(_controllerNotifies); + + /// Route pushes observed since the last session began, oldest first. Each + /// entry carries `route` (the page name magic stamps on its routes, which is + /// the route name or else its path), `durationMicros` (push to the first + /// post-frame callback after the new route built) and `time`. + static List> get routeTransitions => + _routeTransitions.toList(); + + /// Test-only reset. Drops the idempotency guard, clears the magic-side + /// counters, uninstalls the frame watcher, and restores all four dusk + /// pointers to their no-op defaults so a later test asserting the + /// missing-integration behaviour does not see a leaked binding. + /// + /// Also forces wind's counting off: a test that ran the begin hook would + /// otherwise leave `WindParser.parse` instrumented for every later test. + /// + /// Does NOT unregister the observer (a fresh `MagicRouter.reset()` drops it + /// with the router instance) and cannot unregister the watcher from + /// [TelescopePlugin], whose list is private; uninstalling the watcher is + /// what stops it recording. + @visibleForTesting + static void resetForTesting() { + _installed = false; + _controllerNotifies.clear(); + _routeTransitions.clear(); + MagicController.onRefreshUI = null; + _watcher?.uninstall(); + _watcher = null; + WindPerfCounters.enabled = false; + framePerfReader = () => { + 'frames': >[], + 'livenessCounter': 0, + }; + perfExtrasReader = () => { + 'controllerNotifies': {}, + 'routeTransitions': >[], + }; + perfSessionBeginHook = () {}; + perfSessionEndHook = () {}; + } + + static void _recordNotify(MagicController controller) { + _controllerNotifies.update( + controller.runtimeType.toString(), + (int count) => count + 1, + ifAbsent: () => 1, + ); + } + + static void _recordRouteTransition(String route, int durationMicros) { + _routeTransitions.addLast({ + 'route': route, + 'durationMicros': durationMicros, + 'time': DateTime.now().toIso8601String(), + }); + while (_routeTransitions.length > _maxRouteTransitions) { + _routeTransitions.removeFirst(); + } + } + + static bool _installed = false; + static FramePerfWatcher? _watcher; + static final _RouteTransitionObserver _observer = _RouteTransitionObserver(); + static final Map _controllerNotifies = {}; + static final Queue> _routeTransitions = + Queue>(); +} + +/// Times a route push from the moment the navigator reports it to the first +/// post-frame callback after it, which is the first point the new route has +/// actually built and laid out. +/// +/// Only pushes are timed. A pop tears a route down rather than building one, so +/// it has no equivalent span, and go_router replaces the whole page stack on a +/// `go()`, which the navigator reports as a push of the incoming route. +class _RouteTransitionObserver extends NavigatorObserver { + @override + void didPush(Route route, Route? previousRoute) { + super.didPush(route, previousRoute); + + final String name = route.settings.name ?? '(unnamed)'; + final Stopwatch watch = Stopwatch()..start(); + + // One-shot by design, one per push: unlike a per-frame drain there is + // nothing to re-register, because the span closes on the next frame. + SchedulerBinding.instance.addPostFrameCallback((Duration _) { + watch.stop(); + MagicPerfIntegration._recordRouteTransition( + name, + watch.elapsedMicroseconds, + ); + }); + } +} diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart new file mode 100644 index 0000000..db921bc --- /dev/null +++ b/test/perf_integration_test.dart @@ -0,0 +1,292 @@ +import 'dart:ui' show FrameTiming, PlatformDispatcher, TimingsCallback; + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:fluttersdk_dusk/dusk.dart' + show + framePerfReader, + perfExtrasReader, + perfSessionBeginHook, + perfSessionEndHook; +import 'package:fluttersdk_telescope/telescope.dart'; +import 'package:magic/magic.dart'; +import 'package:magic_devtools/magic_devtools.dart'; + +/// Tests for [MagicPerfIntegration], the single place dusk, telescope, wind and +/// magic meet. +/// +/// The failure mode this file exists to prevent is silent: an unassigned reader +/// pointer or an unregistered observer produces a structurally complete report +/// of zeros, with no error, in a different repository. So every test here +/// asserts on the DATA that reaches the pointer, never on `install()` merely +/// returning. + +// --------------------------------------------------------------------------- +// Test fixtures +// --------------------------------------------------------------------------- + +class _AlphaController extends MagicController {} + +class _BetaController extends MagicController {} + +/// Builds a [FrameTiming] from the raw microsecond stamps its public factory +/// takes; that factory's own docstring says it exists for unit tests, and +/// `tester.pump()` delivers no timing of its own. +FrameTiming _timing({required int frameNumber}) { + const int vsyncStart = 0; + const int buildStart = 1000; + const int buildFinish = buildStart + 4000; + const int rasterFinish = buildFinish + 2000; + + return FrameTiming( + vsyncStart: vsyncStart, + buildStart: buildStart, + buildFinish: buildFinish, + rasterStart: buildFinish, + rasterFinish: rasterFinish, + rasterFinishWallTime: rasterFinish, + frameNumber: frameNumber, + ); +} + +/// Fires a timings batch the way the engine would. +/// +/// Asserts the dispatcher is armed first: with no timings callback registered +/// `onReportTimings` is null, and a silently-null `?.call` would make every +/// count below vacuous. +void _fireTimings(List timings) { + final TimingsCallback? report = PlatformDispatcher.instance.onReportTimings; + expect( + report, + isNotNull, + reason: 'the platform dispatcher must be armed for an injected batch to ' + 'reach the frame watcher at all', + ); + report!(timings); +} + +FramePerfRecord _frameRecord(int frameNumber) => FramePerfRecord( + frameNumber: frameNumber, + buildMicros: 4000, + rasterMicros: 2000, + vsyncOverheadMicros: 1000, + totalSpanMicros: 7000, + time: DateTime(2026, 8, 25), + blocks: const {}, +); + +void main() { + setUpAll(() { + TestWidgetsFlutterBinding.ensureInitialized(); + }); + + setUp(() { + MagicApp.reset(); + Magic.flush(); + MagicRouter.reset(); + MagicPerfIntegration.resetForTesting(); + TelescopeStore.resetForTesting(); + }); + + tearDown(() { + MagicPerfIntegration.resetForTesting(); + MagicRouter.reset(); + TelescopeStore.resetForTesting(); + WindPerfCounters.enabled = false; + WindPerfCounters.reset(); + }); + + group('MagicPerfIntegration.install', () { + test('registers exactly one observer and one watcher when called twice', () { + MagicPerfIntegration.install(); + MagicPerfIntegration.install(); + + expect(MagicPerfIntegration.isInstalled, isTrue); + expect(MagicRouter.instance.observers, hasLength(1)); + + // TelescopePlugin keeps its watcher list private, so the watcher count is + // asserted through its only observable effect: a second FramePerfWatcher + // would add a second timings callback and record the same frame twice. + _fireTimings([_timing(frameNumber: 7)]); + expect(TelescopeStore.recentFramePerf(), hasLength(1)); + }); + + test('attributes notify counts to each controller runtime type', () { + MagicPerfIntegration.install(); + + final _AlphaController alpha = _AlphaController(); + final _BetaController beta = _BetaController(); + alpha.refreshUI(); + alpha.refreshUI(); + beta.refreshUI(); + + expect(MagicPerfIntegration.controllerNotifyCounts, { + '_AlphaController': 2, + '_BetaController': 1, + }); + }); + + testWidgets('surfaces the StateError when the router is already built', ( + WidgetTester tester, + ) async { + MagicRoute.page('/', () => const SizedBox()); + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + // A swallowed StateError would leave the report with no route + // transitions and no explanation for why. + expect(MagicPerfIntegration.install, throwsStateError); + expect(MagicPerfIntegration.isInstalled, isFalse); + }); + }); + + group('the dusk pointers', () { + test('framePerfReader returns the recorded frames and the counter', () { + TelescopeStore.recordFramePerf(_frameRecord(11)); + TelescopeStore.recordFramePerf(_frameRecord(12)); + + MagicPerfIntegration.install(); + + final Map payload = framePerfReader(); + expect(payload.keys, unorderedEquals(['frames', 'livenessCounter'])); + + final List frames = payload['frames']! as List; + expect(frames, hasLength(2)); + expect( + frames + .cast>() + .map((Map f) => f['frameNumber']), + [11, 12], + ); + expect(payload['livenessCounter'], isA()); + }); + + test('perfExtrasReader returns the notify counts', () { + MagicPerfIntegration.install(); + _AlphaController().refreshUI(); + + final Map payload = perfExtrasReader(); + expect( + payload.keys, + unorderedEquals(['controllerNotifies', 'routeTransitions']), + ); + expect(payload['controllerNotifies'], {'_AlphaController': 1}); + expect(payload['routeTransitions'], isEmpty); + }); + + testWidgets('perfExtrasReader carries a named, timed route transition', ( + WidgetTester tester, + ) async { + MagicRoute.page('/', () => const SizedBox()); + MagicRoute.page('/monitors', () => const SizedBox()); + + // Before the router is built, which is the whole reason the observer + // registration lives in installPre(). + MagicPerfIntegration.install(); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/monitors'); + await tester.pumpAndSettle(); + + final List transitions = + perfExtrasReader()['routeTransitions']! as List; + expect(transitions, isNotEmpty); + + final Map last = + transitions.last! as Map; + expect(last['route'], '/monitors'); + expect(last['durationMicros'], isA()); + expect(last['durationMicros']! as int, greaterThanOrEqualTo(0)); + }); + + test('perfSessionBeginHook clears only the perf state', () { + WindPerfCounters.enabled = true; + WindPerfCounters.recordCacheHit(); + TelescopeStore.recordFramePerf(_frameRecord(3)); + TelescopeStore.recordDump( + DumpRecord(message: 'sibling buffer', time: DateTime(2026, 8, 25)), + ); + + MagicPerfIntegration.install(); + _AlphaController().refreshUI(); + perfSessionBeginHook(); + + expect(WindPerfCounters.cacheHits, 0); + expect(TelescopeStore.recentFramePerf(), isEmpty); + expect(MagicPerfIntegration.controllerNotifyCounts, isEmpty); + // TelescopeStore.clear() would have taken this with it, which is why the + // hook calls clearFramePerf() instead. + expect( + TelescopeStore.recentDumps().map((DumpRecord r) => r.message), + contains('sibling buffer'), + ); + }); + + test('the session pair turns wind counting on and back off', () { + MagicPerfIntegration.install(); + expect(WindPerfCounters.enabled, isFalse); + + perfSessionBeginHook(); + + // Zeroing without enabling would report a wind section of all zeros + // beside populated frame and magic sections, with no error to say why. + expect(WindPerfCounters.enabled, isTrue); + expect(WindPerfCounters.cacheHits, 0); + + WindPerfCounters.recordCacheHit(); + perfSessionEndHook(); + + // The end hook stops the counting but leaves the totals alone, because + // `perf_end` reads them to build its report. + expect(WindPerfCounters.enabled, isFalse); + expect(WindPerfCounters.cacheHits, 1); + }); + }); + + group('MagicPerfIntegration.resetForTesting', () { + test('restores the hook, the counters and all four pointers', () { + MagicPerfIntegration.install(); + _AlphaController().refreshUI(); + perfSessionBeginHook(); + TelescopeStore.recordFramePerf(_frameRecord(5)); + + MagicPerfIntegration.resetForTesting(); + + expect(MagicPerfIntegration.isInstalled, isFalse); + expect(MagicController.onRefreshUI, isNull); + expect(MagicPerfIntegration.controllerNotifyCounts, isEmpty); + // A reset that left counting on would tax every later test in the suite. + expect(WindPerfCounters.enabled, isFalse); + expect(framePerfReader(), { + 'frames': >[], + 'livenessCounter': 0, + }); + expect(perfExtrasReader(), { + 'controllerNotifies': {}, + 'routeTransitions': >[], + }); + + // The restored hooks are no-ops: the frame buffer survives the begin + // hook and counting stays off after the end hook. + perfSessionBeginHook(); + perfSessionEndHook(); + expect(TelescopeStore.recentFramePerf(), hasLength(1)); + expect(WindPerfCounters.enabled, isFalse); + }); + }); + + group('MagicDevtools.installPre', () { + test('installs the perf integration before the router is built', () { + MagicDevtools.installPre(); + + expect(MagicPerfIntegration.isInstalled, isTrue); + expect(MagicRouter.instance.observers, hasLength(1)); + }); + }); +} From d37c2cb7747d62516fe68a1c276b1bef54642ab8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 11:30:59 +0300 Subject: [PATCH 2/8] style: run the formatter this branch never ran, which every one of these repos gates on CI runs `dart format --output=none --set-exit-if-changed` in both ci.yml and publish.yml here, and this branch had never had the formatter run on it. Ten files across four repos were dirty, so four of the PRs would have gone red on a check that was never part of my verification loop. The omission has a specific cause worth naming. The consumer app these packages were driven from carries a standing rule NOT to run dart format, because its tree predates the current SDK's tall formatter and reformatting rewrites dozens of untouched files. That rule is about that repository. I carried it into these six, where it does not apply: they resolve to short style in-repo and they gate on a zero diff. A project-specific rule applied outside its project. Only branch-introduced files were formatted, so nothing untouched moved. --- test/perf_integration_test.dart | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart index db921bc..7c0b398 100644 --- a/test/perf_integration_test.dart +++ b/test/perf_integration_test.dart @@ -59,7 +59,8 @@ void _fireTimings(List timings) { expect( report, isNotNull, - reason: 'the platform dispatcher must be armed for an injected batch to ' + reason: + 'the platform dispatcher must be armed for an injected batch to ' 'reach the frame watcher at all', ); report!(timings); @@ -150,14 +151,17 @@ void main() { MagicPerfIntegration.install(); final Map payload = framePerfReader(); - expect(payload.keys, unorderedEquals(['frames', 'livenessCounter'])); + expect( + payload.keys, + unorderedEquals(['frames', 'livenessCounter']), + ); final List frames = payload['frames']! as List; expect(frames, hasLength(2)); expect( - frames - .cast>() - .map((Map f) => f['frameNumber']), + frames.cast>().map( + (Map f) => f['frameNumber'], + ), [11, 12], ); expect(payload['livenessCounter'], isA()); @@ -172,7 +176,9 @@ void main() { payload.keys, unorderedEquals(['controllerNotifies', 'routeTransitions']), ); - expect(payload['controllerNotifies'], {'_AlphaController': 1}); + expect(payload['controllerNotifies'], { + '_AlphaController': 1, + }); expect(payload['routeTransitions'], isEmpty); }); From 886c3aab58246ac899df470434f2774455f89e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 15:05:13 +0300 Subject: [PATCH 3/8] fix(perf): arm the guard last, and stop dialogs evicting the transitions being ranked Four review points, all correct. The idempotency guard was armed after the observer registration but before the four steps that follow it, so a throw from Wind.installPerfResolver(), registerWatcher or the pointer assignments left the guard set with the pointers unassigned, and any retry became exactly the silent no-op the class exists to prevent. It is armed at the end now, and the observer gets its own flag so a retry does not register a second one. The original comment argued for the right property and the code only delivered half of it. Route transitions no longer record anonymous pushes. showDialog and showModalBottomSheet go through the same navigator, so in a dialog-heavy session those entries shared the bounded list with real page transitions and could evict the ones the report is ranking. Skipped rather than bucketed: a duration nobody can attribute to a screen is not one an agent can act on. resetForTesting no longer hand-writes dusk's no-op defaults. It captures them at load, before this package assigns over them, and restores those. Re-typing them let this package and its own tests agree on a key set that had drifted from dusk's, so the assertions would have kept passing while production drifted with them. And installPre's docstring gained the caveat it needed. It ends "a second call in the same isolate is safe", which is still true, but a FIRST call made after the router is built now throws. A host installing behind a lazy debug toggle after runApp used to get harmless no-ops. --- lib/src/magic_devtools.dart | 9 ++++++ lib/src/perf_integration.dart | 57 ++++++++++++++++++++++++--------- test/perf_integration_test.dart | 23 +++++++++++++ 3 files changed, 73 insertions(+), 16 deletions(-) diff --git a/lib/src/magic_devtools.dart b/lib/src/magic_devtools.dart index d4b687c..3dc8ab3 100644 --- a/lib/src/magic_devtools.dart +++ b/lib/src/magic_devtools.dart @@ -69,6 +69,15 @@ class MagicDevtools { /// Each underlying install is idempotent, so a second call in the same /// isolate is safe. Register additional watchers after this call via /// [TelescopePlugin.registerWatcher]. + /// + /// It is NOT safe to call late, though, and that is new: the perf + /// integration registers a [NavigatorObserver], and `MagicRouter.addObserver` + /// throws a [StateError] once the router has been built. A host that installs + /// this behind a lazy debug toggle after `runApp` used to get harmless + /// no-ops and now crashes. The throw is deliberate, since a silently + /// unregistered observer would produce a report with no route transitions + /// and nothing to explain their absence, but it means this belongs at boot + /// and nowhere else. static void installPre() { DuskPlugin.install(); diff --git a/lib/src/perf_integration.dart b/lib/src/perf_integration.dart index 22eb7b4..a2cc8db 100644 --- a/lib/src/perf_integration.dart +++ b/lib/src/perf_integration.dart @@ -33,6 +33,16 @@ import 'package:magic/magic.dart'; /// `fluttersdk_wind` is reached through magic's barrel, which re-exports it /// wholesale (`magic/lib/magic.dart:4`); importing it directly here would be /// flagged as an unnecessary import. +/// dusk's own no-op defaults, captured before this package assigns over them. +/// +/// Read at load rather than re-typed in `resetForTesting`, so a change to +/// dusk's declared shape reaches the reset instead of leaving this package and +/// its tests agreeing with each other about a contract that had moved. +final Map Function() _duskFramePerfDefault = framePerfReader; +final Map Function() _duskPerfExtrasDefault = perfExtrasReader; +final void Function() _duskSessionBeginDefault = perfSessionBeginHook; +final void Function() _duskSessionEndDefault = perfSessionEndHook; + class MagicPerfIntegration { MagicPerfIntegration._(); @@ -53,11 +63,15 @@ class MagicPerfIntegration { static void install() { if (_installed) return; - // 1. The only step that can fail, so it runs before the idempotency guard - // is armed: marking the integration installed and then throwing would - // turn a retry into a silent no-op. - MagicRouter.instance.addObserver(_observer); - _installed = true; + // 1. Registered once and tracked separately, because the guard below is + // armed at the END rather than here. Arming it early would make a retry + // after a throw in steps 2 to 4 a silent no-op, which is the failure + // this class exists to prevent; arming it late without this flag would + // register a second observer on that retry. + if (!_observerRegistered) { + MagicRouter.instance.addObserver(_observer); + _observerRegistered = true; + } // 2. magic: one hook on the single notifyListeners() call site in // MagicController, counted per controller runtime type so the report can @@ -102,6 +116,9 @@ class MagicPerfIntegration { // report, and `WindParser.parse` is too hot to leave instrumented. WindPerfCounters.enabled = false; }; + + // 5. Last, so a throw anywhere above leaves the door open for a retry. + _installed = true; } /// Whether [install] has been called at least once. @@ -135,22 +152,21 @@ class MagicPerfIntegration { @visibleForTesting static void resetForTesting() { _installed = false; + _observerRegistered = false; _controllerNotifies.clear(); _routeTransitions.clear(); MagicController.onRefreshUI = null; _watcher?.uninstall(); _watcher = null; WindPerfCounters.enabled = false; - framePerfReader = () => { - 'frames': >[], - 'livenessCounter': 0, - }; - perfExtrasReader = () => { - 'controllerNotifies': {}, - 'routeTransitions': >[], - }; - perfSessionBeginHook = () {}; - perfSessionEndHook = () {}; + // Restored from the values dusk itself declared, captured once at load, + // rather than hand-written here. Re-typing them would let this package and + // its tests agree on a key set that had drifted from dusk's, and the + // assertions would keep passing while production drifted with them. + framePerfReader = _duskFramePerfDefault; + perfExtrasReader = _duskPerfExtrasDefault; + perfSessionBeginHook = _duskSessionBeginDefault; + perfSessionEndHook = _duskSessionEndDefault; } static void _recordNotify(MagicController controller) { @@ -173,6 +189,7 @@ class MagicPerfIntegration { } static bool _installed = false; + static bool _observerRegistered = false; static FramePerfWatcher? _watcher; static final _RouteTransitionObserver _observer = _RouteTransitionObserver(); static final Map _controllerNotifies = {}; @@ -192,7 +209,15 @@ class _RouteTransitionObserver extends NavigatorObserver { void didPush(Route route, Route? previousRoute) { super.didPush(route, previousRoute); - final String name = route.settings.name ?? '(unnamed)'; + // An anonymous push is a dialog or a bottom sheet, not a page transition. + // showDialog and showModalBottomSheet both go through the navigator, so in + // a dialog-heavy session they would share the bounded list with the real + // transitions and evict the very entries the report is ranking. Skipped + // rather than bucketed: a duration nobody can attribute to a screen is not + // one an agent can act on. + final String? name = route.settings.name; + if (name == null) return; + final Stopwatch watch = Stopwatch()..start(); // One-shot by design, one per push: unlike a per-frame drain there is diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart index 7c0b398..db5f63d 100644 --- a/test/perf_integration_test.dart +++ b/test/perf_integration_test.dart @@ -295,4 +295,27 @@ void main() { expect(MagicRouter.instance.observers, hasLength(1)); }); }); + group('route transitions', () { + test('an anonymous push is not recorded', () { + // showDialog and showModalBottomSheet push unnamed routes through the + // same navigator. Recording them would let a dialog-heavy session evict + // the real page transitions out of the bounded list the report ranks. + MagicPerfIntegration.install(); + addTearDown(MagicPerfIntegration.resetForTesting); + + final int before = MagicPerfIntegration.routeTransitions.length; + + MagicRouter.instance.observers.first.didPush( + PageRouteBuilder( + pageBuilder: (_, __, ___) => const SizedBox.shrink(), + ), + null, + ); + + // No pump needed and that is the point: an unnamed push returns before + // scheduling the post-frame callback that would close the span, so + // there is nothing in flight to wait for. + expect(MagicPerfIntegration.routeTransitions, hasLength(before)); + }); + }); } From 3ba45983438c0eeedf651546452b2851b1688a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 15:34:00 +0300 Subject: [PATCH 4/8] fix(perf): a top-level final captures on first read, not at load The previous commit claimed to capture dusk's defaults "before this package assigns over them" and did not. A top-level `final` in Dart initialises on first READ, and the only reader is resetForTesting(), which runs after install() has already overwritten the pointers. So it captured this package's own closures and restored those, which means every "back to the default" assertion was really asserting that install had happened. Verified with a standalone repro of the pattern before replacing it: the reset returned INTEGRATION, not the default. A comment asserting a language behaviour the language does not have is worse than no comment, because it stops the next reader checking. Captured eagerly now, at the top of the pointer assignments, which is the last moment they are still readable. The nullable fields make "install never ran" representable, and in that case there is nothing to put back. The two tests that moved wind's counters through record* now build a real WDiv instead. Those entry points are @internal to fluttersdk_wind, so reaching for them asserted against a surface no consumer is meant to touch, and a pump is the honest version of that setup anyway: it is what moves these numbers in an app. Mutation-checked: removing the capture turns the reset test red. --- analysis_options.yaml | 8 ++++++ lib/src/perf_integration.dart | 44 ++++++++++++++++++++++++--------- test/perf_integration_test.dart | 29 +++++++++++++++++++--- 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/analysis_options.yaml b/analysis_options.yaml index d070858..6cd226c 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -5,6 +5,14 @@ analyzer: # List as descriptive placeholders, not HTML. Mirrors magic's own # analysis_options (these files were extracted from magic verbatim). unintended_html_in_doc_comment: ignore + exclude: + - build/** + - android/** + - ios/** + - web/** + - windows/** + - macos/** + - linux/** # Additional information about this file can be found at # https://dart.dev/guides/language/analysis-options diff --git a/lib/src/perf_integration.dart b/lib/src/perf_integration.dart index a2cc8db..0adafc6 100644 --- a/lib/src/perf_integration.dart +++ b/lib/src/perf_integration.dart @@ -33,15 +33,24 @@ import 'package:magic/magic.dart'; /// `fluttersdk_wind` is reached through magic's barrel, which re-exports it /// wholesale (`magic/lib/magic.dart:4`); importing it directly here would be /// flagged as an unnecessary import. -/// dusk's own no-op defaults, captured before this package assigns over them. +/// dusk's own no-op defaults, captured the first time [MagicPerfIntegration] +/// is about to overwrite them. /// -/// Read at load rather than re-typed in `resetForTesting`, so a change to -/// dusk's declared shape reaches the reset instead of leaving this package and -/// its tests agreeing with each other about a contract that had moved. -final Map Function() _duskFramePerfDefault = framePerfReader; -final Map Function() _duskPerfExtrasDefault = perfExtrasReader; -final void Function() _duskSessionBeginDefault = perfSessionBeginHook; -final void Function() _duskSessionEndDefault = perfSessionEndHook; +/// Captured rather than re-typed here, so a change to dusk's declared shape +/// reaches the reset instead of leaving this package and its tests agreeing +/// with each other about a contract that had moved. +/// +/// NOT top-level `final`s, which is the version this replaces and which did +/// not work: a top-level `final` in Dart initialises on first READ, and the +/// only reader is `resetForTesting()`, which runs after `install()` has +/// already assigned over the pointers. It captured this package's own closures +/// and restored them, so every "back to the default" assertion was really +/// asserting that install had happened. Verified with a standalone repro +/// before replacing it. +Map Function()? _duskFramePerfDefault; +Map Function()? _duskPerfExtrasDefault; +void Function()? _duskSessionBeginDefault; +void Function()? _duskSessionEndDefault; class MagicPerfIntegration { MagicPerfIntegration._(); @@ -89,6 +98,13 @@ class MagicPerfIntegration { // 4. The four dusk pointers. Each returns exactly the key set pinned in // `dusk/lib/src/utils/perf_readers.dart`; the consumer is in another // repository, so a renamed key is invisible until a driven run. + // + // dusk's own defaults are captured HERE, immediately before they are + // overwritten, because that is the last moment they are still readable. + _duskFramePerfDefault ??= framePerfReader; + _duskPerfExtrasDefault ??= perfExtrasReader; + _duskSessionBeginDefault ??= perfSessionBeginHook; + _duskSessionEndDefault ??= perfSessionEndHook; framePerfReader = () => { 'frames': TelescopeStore.recentFramePerf() .map>((FramePerfRecord r) => r.toJson()) @@ -163,10 +179,14 @@ class MagicPerfIntegration { // rather than hand-written here. Re-typing them would let this package and // its tests agree on a key set that had drifted from dusk's, and the // assertions would keep passing while production drifted with them. - framePerfReader = _duskFramePerfDefault; - perfExtrasReader = _duskPerfExtrasDefault; - perfSessionBeginHook = _duskSessionBeginDefault; - perfSessionEndHook = _duskSessionEndDefault; + // Null only when install() never ran, in which case the pointers are + // already at dusk's defaults and there is nothing to put back. + if (_duskFramePerfDefault != null) { + framePerfReader = _duskFramePerfDefault!; + perfExtrasReader = _duskPerfExtrasDefault!; + perfSessionBeginHook = _duskSessionBeginDefault!; + perfSessionEndHook = _duskSessionEndDefault!; + } } static void _recordNotify(MagicController controller) { diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart index db5f63d..1a3f483 100644 --- a/test/perf_integration_test.dart +++ b/test/perf_integration_test.dart @@ -76,6 +76,23 @@ FramePerfRecord _frameRecord(int frameNumber) => FramePerfRecord( blocks: const {}, ); +/// Moves wind's counters the way the app does: by building a real W-widget. +/// +/// The `record*` entry points are `@internal` to `fluttersdk_wind`, since they +/// exist for its own parse path, so reaching for them here would assert +/// against a surface no consumer is meant to touch. A pump is also the honest +/// version of this setup: it is what actually moves these numbers in an app. +Future _buildOneWidget(WidgetTester tester) async { + await tester.pumpWidget( + MaterialApp( + home: WindTheme( + data: WindThemeData(), + child: const WDiv(className: 'p-4'), + ), + ), + ); +} + void main() { setUpAll(() { TestWidgetsFlutterBinding.ensureInitialized(); @@ -211,9 +228,11 @@ void main() { expect(last['durationMicros']! as int, greaterThanOrEqualTo(0)); }); - test('perfSessionBeginHook clears only the perf state', () { + testWidgets('perfSessionBeginHook clears only the perf state', ( + WidgetTester tester, + ) async { WindPerfCounters.enabled = true; - WindPerfCounters.recordCacheHit(); + await _buildOneWidget(tester); TelescopeStore.recordFramePerf(_frameRecord(3)); TelescopeStore.recordDump( DumpRecord(message: 'sibling buffer', time: DateTime(2026, 8, 25)), @@ -234,7 +253,9 @@ void main() { ); }); - test('the session pair turns wind counting on and back off', () { + testWidgets('the session pair turns wind counting on and back off', ( + WidgetTester tester, + ) async { MagicPerfIntegration.install(); expect(WindPerfCounters.enabled, isFalse); @@ -245,7 +266,7 @@ void main() { expect(WindPerfCounters.enabled, isTrue); expect(WindPerfCounters.cacheHits, 0); - WindPerfCounters.recordCacheHit(); + await _buildOneWidget(tester); perfSessionEndHook(); // The end hook stops the counting but leaves the totals alone, because From 4f42c588f4ed649b124bd6339b7c0fa7c9b8f787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 22:23:49 +0300 Subject: [PATCH 5/8] test(perf): pin the parse-cache state, and stop leaking debugPrint Review found the two session tests coupled to wind's static parse cache, and reproducing it turned up a second order dependence underneath. WindParser._styleCache is a static map shared across the isolate, and both tests built the same WDiv under the same theme, so they shared one key. The first ran against a cold cache, which made its `expect(cacheHits, 0)` assert a value that was already 0 and prove nothing about the reset it was there to check; the second only passed because the first had warmed the key. Verified: `--plain-name 'the session pair'` alone was red with Expected 1, Actual 0. Each test now warms the cache itself through a helper that clears it, builds, and zeroes the counters. The first also asserts a non-zero BEFORE the hook, so the reset has something to have cleared. The helper pumps a different tree between the two builds: pumping an identical tree does not rebuild it, so the measured build parsed nothing and the hit never happened. That cost a wrong first attempt. The second dependence: MagicDevtools.installPre() installs telescope's DumpWatcher, which replaces the global debugPrint, and nothing put it back. So Flutter's own post-test check fired on whichever testWidgets case ran NEXT, blaming an innocent test under only some orderings. tearDown restores it now. The whole suite is green under five shuffle seeds; before, seed 12345 was red. Also corrects the restore comment, which still said the defaults are "captured once at load" after the fix moved that to first install, and drops the analysis_options exclude block: none of those seven directories exist in this package, so it excluded nothing and was unrelated to the perf path. --- analysis_options.yaml | 8 -------- lib/src/perf_integration.dart | 5 ++++- test/perf_integration_test.dart | 36 +++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/analysis_options.yaml b/analysis_options.yaml index 6cd226c..d070858 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -5,14 +5,6 @@ analyzer: # List as descriptive placeholders, not HTML. Mirrors magic's own # analysis_options (these files were extracted from magic verbatim). unintended_html_in_doc_comment: ignore - exclude: - - build/** - - android/** - - ios/** - - web/** - - windows/** - - macos/** - - linux/** # Additional information about this file can be found at # https://dart.dev/guides/language/analysis-options diff --git a/lib/src/perf_integration.dart b/lib/src/perf_integration.dart index 0adafc6..eb71fa9 100644 --- a/lib/src/perf_integration.dart +++ b/lib/src/perf_integration.dart @@ -175,7 +175,10 @@ class MagicPerfIntegration { _watcher?.uninstall(); _watcher = null; WindPerfCounters.enabled = false; - // Restored from the values dusk itself declared, captured once at load, + // Restored from the values dusk itself declared, captured on the first + // install rather than at load (a top-level `final` in Dart initialises on + // first READ, so capturing at load would have caught this integration's own + // closures instead of dusk's defaults), // rather than hand-written here. Re-typing them would let this package and // its tests agree on a key set that had drifted from dusk's, and the // assertions would keep passing while production drifted with them. diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart index 1a3f483..4a48e7f 100644 --- a/test/perf_integration_test.dart +++ b/test/perf_integration_test.dart @@ -93,12 +93,39 @@ Future _buildOneWidget(WidgetTester tester) async { ); } +/// Builds the widget once against a COLD parse cache, so the build that +/// follows is guaranteed to be a hit. +/// +/// `WindParser._styleCache` is a static map keyed by className plus theme +/// state, so every test in this isolate shares it. Without pinning it, whether +/// a build counts as a hit or a miss depends on which test ran first: these two +/// cases used to pass together and fail when either was run alone, because one +/// warmed the key the other asserted on. +Future _warmTheParseCache(WidgetTester tester) async { + WindParser.clearCache(); + await _buildOneWidget(tester); + // Pump something else in between: pumping an identical tree does not rebuild + // it, so the next _buildOneWidget would parse nothing at all and the hit the + // caller is waiting for would never happen. + await tester.pumpWidget(const SizedBox.shrink()); + WindPerfCounters.reset(); +} + void main() { setUpAll(() { TestWidgetsFlutterBinding.ensureInitialized(); }); + // `MagicDevtools.installPre()` installs telescope's DumpWatcher, which + // replaces the global `debugPrint`. Nothing here used to put it back, so + // Flutter's own post-test check ("the value of a foundation debug variable + // was changed by the test") fired on whichever `testWidgets` case happened to + // run NEXT. That made the failure land on an innocent test and only under + // some orderings, which is why it survived a green suite. + late void Function(String?, {int? wrapWidth}) originalDebugPrint; + setUp(() { + originalDebugPrint = debugPrint; MagicApp.reset(); Magic.flush(); MagicRouter.reset(); @@ -112,6 +139,7 @@ void main() { TelescopeStore.resetForTesting(); WindPerfCounters.enabled = false; WindPerfCounters.reset(); + debugPrint = originalDebugPrint; }); group('MagicPerfIntegration.install', () { @@ -232,7 +260,12 @@ void main() { WidgetTester tester, ) async { WindPerfCounters.enabled = true; + await _warmTheParseCache(tester); await _buildOneWidget(tester); + // A non-zero before the hook runs is the whole point: asserting zero + // afterwards proves nothing if it was already zero, which is what this + // case did while it happened to run first. + expect(WindPerfCounters.cacheHits, greaterThan(0)); TelescopeStore.recordFramePerf(_frameRecord(3)); TelescopeStore.recordDump( DumpRecord(message: 'sibling buffer', time: DateTime(2026, 8, 25)), @@ -256,6 +289,9 @@ void main() { testWidgets('the session pair turns wind counting on and back off', ( WidgetTester tester, ) async { + // Warmed here rather than inherited from whichever test ran before, so + // the build below is a hit whatever the order or the shuffle seed. + await _warmTheParseCache(tester); MagicPerfIntegration.install(); expect(WindPerfCounters.enabled, isFalse); From 077abf6e7bf972bd8a709d16ff98a8f12af222d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 22:34:04 +0300 Subject: [PATCH 6/8] test(perf): restore debugPrint inside the test body, and keep the generated yaml Two review notes, both verified against the source before acting. The debugPrint restore was in tearDown, which runs AFTER Flutter's end-of-test check: AutomatedTestWidgetsFlutterBinding.runTest calls _verifyInvariants() immediately after `await testBody()` (flutter_test/lib/src/binding.dart:1974), so tearDown and addTearDown are both too late. It passed only because the sole installPre() call sits in a plain test(), which has no invariant check. Proven by converting that case to testWidgets as a probe: it failed on itself with "the value of a foundation debug variable was changed by the test". The restore now happens inline at the end of that body. Re-running the same probe, the foundation error is gone. What surfaces instead is a SECOND end-of-test invariant: installPre() leaves a SemanticsHandle active, because dusk's snapshot pipeline enables semantics. That is latent today, since the case is a plain test(), so it is recorded as a note rather than fixed here. The tearDown restore stays as a net for the plain test() cases. Reverting the analysis_options.yaml removal. The exclude block is written by `flutter pub get` ("Upgrading analysis_options.yaml to exclude build and platform directories"), reproduced here, so removing it just makes every tree dirty after a dependency fetch. My earlier note that it was unrelated to this PR was right for the wrong reason: it is unrelated because Flutter generated it, which is exactly why it should stay. Suite green under three shuffle seeds, 110 tests each. --- analysis_options.yaml | 8 ++++++++ test/perf_integration_test.dart | 31 ++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/analysis_options.yaml b/analysis_options.yaml index d070858..6cd226c 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -5,6 +5,14 @@ analyzer: # List as descriptive placeholders, not HTML. Mirrors magic's own # analysis_options (these files were extracted from magic verbatim). unintended_html_in_doc_comment: ignore + exclude: + - build/** + - android/** + - ios/** + - web/** + - windows/** + - macos/** + - linux/** # Additional information about this file can be found at # https://dart.dev/guides/language/analysis-options diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart index 4a48e7f..b160551 100644 --- a/test/perf_integration_test.dart +++ b/test/perf_integration_test.dart @@ -117,13 +117,24 @@ void main() { }); // `MagicDevtools.installPre()` installs telescope's DumpWatcher, which - // replaces the global `debugPrint`. Nothing here used to put it back, so - // Flutter's own post-test check ("the value of a foundation debug variable - // was changed by the test") fired on whichever `testWidgets` case happened to - // run NEXT. That made the failure land on an innocent test and only under - // some orderings, which is why it survived a green suite. + // replaces the global `debugPrint`. Nothing used to put it back, so Flutter's + // own post-test check ("the value of a foundation debug variable was changed + // by the test") fired on whichever `testWidgets` case happened to run NEXT. + // That made the failure land on an innocent test and only under some + // orderings, which is why it survived a green suite. + // + // The restore has to happen INSIDE the test body, which is what + // [_restoreDebugPrint] is for. `_verifyInvariants()` runs immediately after + // `await testBody()` in `AutomatedTestWidgetsFlutterBinding.runTest` + // (`flutter_test/lib/src/binding.dart:1974`), so both `tearDown` and + // `addTearDown` are too late: a `testWidgets` case that installs the watcher + // fails on ITSELF before either runs. The `tearDown` below is a net for the + // plain `test()` cases, which have no invariant check. late void Function(String?, {int? wrapWidth}) originalDebugPrint; + /// Puts the global `debugPrint` back, from inside the test body. + void restoreDebugPrint() => debugPrint = originalDebugPrint; + setUp(() { originalDebugPrint = debugPrint; MagicApp.reset(); @@ -347,9 +358,19 @@ void main() { group('MagicDevtools.installPre', () { test('installs the perf integration before the router is built', () { MagicDevtools.installPre(); + // Inline, not in a tearDown: see the note on [restoreDebugPrint]. This + // case is a plain `test()` today, so it would survive either way, but + // the day it becomes a `testWidgets` it would fail on itself. + // + // If you do convert it, note that `installPre()` also leaves a + // SemanticsHandle active (dusk's snapshot pipeline enables semantics), + // which is a second end-of-test invariant and needs its own dispose. + // Measured by converting this case as a probe. + addTearDown(restoreDebugPrint); expect(MagicPerfIntegration.isInstalled, isTrue); expect(MagicRouter.instance.observers, hasLength(1)); + restoreDebugPrint(); }); }); group('route transitions', () { From 17a472408122a80f9d7daeacb568947a79f24075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 23:02:23 +0300 Subject: [PATCH 7/8] docs(perf): unfuse the class docblock from the four pointer defaults The block describing MagicPerfIntegration and the block describing the four captured dusk defaults had merged into one, sitting on the private top-level variables. The tell is mid-paragraph: a sentence about reaching wind through magic's barrel is followed, with no break, by "dusk's own no-op defaults, captured the first time...", so the class's own documentation read as an aside about four private fields and the class itself carried none. Split back to where each belongs. The class doc is on the class, the pointer doc is on the pointers. resetForTesting's comment went with it. It had absorbed a copy of the top-level final explanation inside a parenthetical, which left the surrounding sentence reading "captured on the first install rather than at load (...), rather than hand-written here". It now states its own point and refers to the fields for the rest. Comments only, no behaviour change. --- lib/src/perf_integration.dart | 51 +++++++++++++++++------------------ 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/src/perf_integration.dart b/lib/src/perf_integration.dart index eb71fa9..1be2090 100644 --- a/lib/src/perf_integration.dart +++ b/lib/src/perf_integration.dart @@ -11,6 +11,25 @@ import 'package:fluttersdk_dusk/dusk.dart' import 'package:fluttersdk_telescope/telescope.dart'; import 'package:magic/magic.dart'; +/// dusk's own no-op defaults, captured the first time [MagicPerfIntegration] +/// is about to overwrite them. +/// +/// Captured rather than re-typed here, so a change to dusk's declared shape +/// reaches the reset instead of leaving this package and its tests agreeing +/// with each other about a contract that had moved. +/// +/// NOT top-level `final`s, which is the version this replaces and which did +/// not work: a top-level `final` in Dart initialises on first READ, and the +/// only reader is `resetForTesting()`, which runs after `install()` has +/// already assigned over the pointers. It captured this package's own closures +/// and restored them, so every "back to the default" assertion was really +/// asserting that install had happened. Verified with a standalone repro +/// before replacing it. +Map Function()? _duskFramePerfDefault; +Map Function()? _duskPerfExtrasDefault; +void Function()? _duskSessionBeginDefault; +void Function()? _duskSessionEndDefault; + /// Assembles the whole performance-diagnostic data path: magic's controller and /// route activity, wind's aggregate counters, telescope's frame buffer, and the /// four pointers `fluttersdk_dusk` reads them all through. @@ -33,25 +52,6 @@ import 'package:magic/magic.dart'; /// `fluttersdk_wind` is reached through magic's barrel, which re-exports it /// wholesale (`magic/lib/magic.dart:4`); importing it directly here would be /// flagged as an unnecessary import. -/// dusk's own no-op defaults, captured the first time [MagicPerfIntegration] -/// is about to overwrite them. -/// -/// Captured rather than re-typed here, so a change to dusk's declared shape -/// reaches the reset instead of leaving this package and its tests agreeing -/// with each other about a contract that had moved. -/// -/// NOT top-level `final`s, which is the version this replaces and which did -/// not work: a top-level `final` in Dart initialises on first READ, and the -/// only reader is `resetForTesting()`, which runs after `install()` has -/// already assigned over the pointers. It captured this package's own closures -/// and restored them, so every "back to the default" assertion was really -/// asserting that install had happened. Verified with a standalone repro -/// before replacing it. -Map Function()? _duskFramePerfDefault; -Map Function()? _duskPerfExtrasDefault; -void Function()? _duskSessionBeginDefault; -void Function()? _duskSessionEndDefault; - class MagicPerfIntegration { MagicPerfIntegration._(); @@ -175,13 +175,12 @@ class MagicPerfIntegration { _watcher?.uninstall(); _watcher = null; WindPerfCounters.enabled = false; - // Restored from the values dusk itself declared, captured on the first - // install rather than at load (a top-level `final` in Dart initialises on - // first READ, so capturing at load would have caught this integration's own - // closures instead of dusk's defaults), - // rather than hand-written here. Re-typing them would let this package and - // its tests agree on a key set that had drifted from dusk's, and the - // assertions would keep passing while production drifted with them. + // Restored from the values dusk itself declared rather than hand-written + // here. Re-typing them would let this package and its tests agree on a key + // set that had drifted from dusk's, and the assertions would keep passing + // while production drifted with them. See the four fields at the top of + // this file for why they are captured on the first install and not at load. + // // Null only when install() never ran, in which case the pointers are // already at dusk's defaults and there is nothing to put back. if (_duskFramePerfDefault != null) { From 0a0ac3d5f5f5fd01d8768fd2cfd8dfb837d3ac55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Tue, 25 Aug 2026 23:22:04 +0300 Subject: [PATCH 8/8] docs(test): fix the dangling doc link and the comment that described the wrong line Both carried over from the previous review round. The note explaining why the restore runs inside the test body pointed at [_restoreDebugPrint]; the function is declared nine lines below as restoreDebugPrint, with no underscore, and the call sites spell it that way too. The analyzer does not resolve doc links to a local function, so nothing was going to catch it, in the one comment whose job is to point at the fix. In the installPre case, "Inline, not in a tearDown" sat directly above an addTearDown call, which is the opposite of what it says. The inline restore is the last line of that body and the tearDown is a net; the comment now says so in that order. Also collapsed `(_, __, ___)` to `(_, _, _)` in the pageBuilder, the two unnecessary_underscores infos CI reports on this branch. They do not fail the gate (analyze runs --no-fatal-infos here) but they are the only two in the package. Comments and wildcard names only; `flutter test` is 110 green. --- test/perf_integration_test.dart | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/perf_integration_test.dart b/test/perf_integration_test.dart index b160551..cbd492a 100644 --- a/test/perf_integration_test.dart +++ b/test/perf_integration_test.dart @@ -124,7 +124,7 @@ void main() { // orderings, which is why it survived a green suite. // // The restore has to happen INSIDE the test body, which is what - // [_restoreDebugPrint] is for. `_verifyInvariants()` runs immediately after + // [restoreDebugPrint] is for. `_verifyInvariants()` runs immediately after // `await testBody()` in `AutomatedTestWidgetsFlutterBinding.runTest` // (`flutter_test/lib/src/binding.dart:1974`), so both `tearDown` and // `addTearDown` are too late: a `testWidgets` case that installs the watcher @@ -358,9 +358,11 @@ void main() { group('MagicDevtools.installPre', () { test('installs the perf integration before the router is built', () { MagicDevtools.installPre(); - // Inline, not in a tearDown: see the note on [restoreDebugPrint]. This - // case is a plain `test()` today, so it would survive either way, but - // the day it becomes a `testWidgets` it would fail on itself. + // A net, not the mechanism. The restore that matters is the inline call + // at the end of this body, for the reason on [restoreDebugPrint]; this + // case is a plain `test()` today, so a tearDown would serve either way, + // but the day it becomes a `testWidgets` it would fail on itself before + // any tearDown runs. // // If you do convert it, note that `installPre()` also leaves a // SemanticsHandle active (dusk's snapshot pipeline enables semantics), @@ -385,7 +387,7 @@ void main() { MagicRouter.instance.observers.first.didPush( PageRouteBuilder( - pageBuilder: (_, __, ___) => const SizedBox.shrink(), + pageBuilder: (_, _, _) => const SizedBox.shrink(), ), null, );