From efd47846aaea4a8fe737ebd65263e4197186f0cf Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Mon, 7 Sep 2026 23:50:56 +0300 Subject: [PATCH 1/2] test(routing): pin queryParameter, which a report claimed was broken A consumer put `?scale=5000` on a route, could not read it back, and filed it against this package. The evidence was `dusk:get_routes` reporting `location: /` while the browser sat on `#/?scale=5000`. That evidence measures something else. Dusk's `location` is `route.settings.name` off the Navigator (`ext_navigation.dart:442`), which under `MaterialApp.router` is the declared path pattern, not the resolved URI. It was always going to read `/` and says nothing about what this router saw. These six tests settle it, and all six passed on the first run: the router reads a query parameter correctly, both when it is passed through the `queryParameters` argument and when it is written inline in the path, which is the shape `to('/?scale=5000')` produces and the one the report used. So there is nothing to fix here, and the report is withdrawn. They stay because the API had no regression test at all, and because the next person who doubts it should be able to settle it in one command rather than by reasoning about GoRouter. The fourth one is the interesting case: a later navigation with no query has to CLEAR the value rather than leave a stale one a caller would read as current. Tests only, no `lib/` change, so the post-change sync does not apply and there is no CHANGELOG entry: nothing about the behaviour moved. --- .../magic_router_query_parameter_test.dart | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 test/routing/magic_router_query_parameter_test.dart diff --git a/test/routing/magic_router_query_parameter_test.dart b/test/routing/magic_router_query_parameter_test.dart new file mode 100644 index 0000000..c0accdb --- /dev/null +++ b/test/routing/magic_router_query_parameter_test.dart @@ -0,0 +1,125 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:magic/magic.dart'; + +/// Tests for [MagicRouter.queryParameter] and [MagicRouter.queryParameters]. +/// +/// Written to settle a report rather than to add coverage. A consumer put a +/// query parameter on a route, read it back through `queryParameter`, got +/// null, and filed it against this package. The evidence turned out to be a +/// misreading: the value they were looking at came from `dusk:get_routes`, +/// whose `location` is `route.settings.name` off the Navigator (the declared +/// path pattern) rather than anything this router exposes. +/// +/// So these pin what the router actually promises, both ways a caller can put +/// a query on a location: the `queryParameters` argument, and inline in the +/// path string, which is the shape `MagicRoute.to('/?scale=5000')` produces +/// and the one the report used. +void main() { + setUpAll(() { + TestWidgetsFlutterBinding.ensureInitialized(); + }); + + setUp(() { + MagicApp.reset(); + Magic.flush(); + TitleManager.reset(); + MagicRouter.reset(); + }); + + group('MagicRouter.queryParameter', () { + testWidgets('reads a parameter passed through queryParameters', ( + tester, + ) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/', queryParameters: {'scale': '5000'}); + await tester.pumpAndSettle(); + + expect(MagicRouter.instance.queryParameter('scale'), '5000'); + }); + + testWidgets('reads a parameter written inline in the path', (tester) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/?scale=5000'); + await tester.pumpAndSettle(); + + expect(MagicRouter.instance.queryParameter('scale'), '5000'); + }); + + testWidgets('is null for a key the location does not carry', ( + tester, + ) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/?scale=5000'); + await tester.pumpAndSettle(); + + expect(MagicRouter.instance.queryParameter('missing'), isNull); + }); + + testWidgets('survives a navigation that carries no query at all', ( + tester, + ) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + MagicRoute.page('/profile', () => const SizedBox()).name('profile'); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/?scale=5000'); + await tester.pumpAndSettle(); + expect(MagicRouter.instance.queryParameter('scale'), '5000'); + + // The query belongs to the LOCATION, not to the session: navigating + // somewhere without one has to clear it rather than leave a stale value + // a caller would read as still current. + MagicRouter.instance.to('/profile'); + await tester.pumpAndSettle(); + expect(MagicRouter.instance.queryParameter('scale'), isNull); + }); + }); + + group('MagicRouter.queryParameters', () { + testWidgets('exposes every parameter on the location', (tester) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/?scale=5000&width=1440'); + await tester.pumpAndSettle(); + + expect(MagicRouter.instance.queryParameters, { + 'scale': '5000', + 'width': '1440', + }); + }); + + testWidgets('is empty rather than null before any route resolves', ( + tester, + ) async { + expect(MagicRouter.instance.queryParameters, isEmpty); + }); + }); +} From 2b74fb5f65045e3c2c4ce7e2df5d7b4ad9f767cf Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Tue, 8 Sep 2026 00:04:42 +0300 Subject: [PATCH 2/2] test(routing): fix a backwards test name, and cover the Request delegation Three points from review. One test name read backwards from what it asserts. "survives a navigation that carries no query at all" says the value persists; the body, the inline comment and the pull request description all say it is cleared, which is the behaviour worth pinning. Renamed to say so. The pre-resolution case pumped nothing and took a `tester` it never used. `.claude/rules/tests.md:18` asks for `test()` for pure logic, and the sibling file already does exactly that for its own no-route-yet case. Now a `test()`. `Request.query` and `Request.queryParams` (`request.dart:72` and `:83`) are one-line delegations to the two accessors this file covers, and nothing tested them either. They are the surface a consumer actually reads, and reading through them is what the original report was doing, so the pairing is the part worth having under test rather than the one-liners themselves. --- .../magic_router_query_parameter_test.dart | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/test/routing/magic_router_query_parameter_test.dart b/test/routing/magic_router_query_parameter_test.dart index c0accdb..6659326 100644 --- a/test/routing/magic_router_query_parameter_test.dart +++ b/test/routing/magic_router_query_parameter_test.dart @@ -74,7 +74,7 @@ void main() { expect(MagicRouter.instance.queryParameter('missing'), isNull); }); - testWidgets('survives a navigation that carries no query at all', ( + testWidgets('clears when a later navigation carries no query', ( tester, ) async { MagicRoute.page('/', () => const SizedBox()).name('home'); @@ -116,10 +116,29 @@ void main() { }); }); - testWidgets('is empty rather than null before any route resolves', ( - tester, - ) async { + test('is empty rather than null before any route resolves', () { expect(MagicRouter.instance.queryParameters, isEmpty); }); }); + + group('Request, which is how a consumer reaches these', () { + testWidgets('query and queryParams delegate to the router', (tester) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRouter.instance.to('/?scale=5000&width=1440'); + await tester.pumpAndSettle(); + + // One line each (`request.dart:72` and `:83`), so this is not really + // testing logic. It is testing that the facade an app reads through is + // wired to the accessor above, which is the pairing the original report + // was about and the one nothing covered. + expect(Request.query('scale'), '5000'); + expect(Request.queryParams, {'scale': '5000', 'width': '1440'}); + }); + }); }