[go_router] Add regression test for synchronous onEnter Block.then(router.go) (flutter#179370)#11725
Draft
NadeemIqbal wants to merge 2 commits into
Draft
Conversation
…uter.go) A synchronous onEnter callback that returns `Block.then(() => router.go(...))` on the initial route must still run the `then` redirect. This was the user-visible symptom in flutter/flutter#179370: replacing `async` with a synchronous callback caused the initial redirect to be silently lost, which the reporter worked around by re-adding `async`. The underlying re-entrancy bug was fixed by flutter#11136 (replacing `await Future<void>.sync(callback)` with `scheduleMicrotask(() async { await callback(); })`). Lock in that behavior with a regression test mirroring the exact predicate from the report (`current.uri.path == '/dashboard'`), which on the initial parse matches because the router has no committed configuration yet. Verified that reverting `parser.dart` to the pre-flutter#11136 implementation makes this test fail; with the current implementation it passes alongside the rest of `on_enter_test.dart` (32/32). Fixes flutter/flutter#179370.
There was a problem hiding this comment.
Code Review
This pull request adds a regression test for issue #179370 in packages/go_router/test/on_enter_test.dart to verify that synchronous onEnter redirects on the initial route function correctly. The reviewer suggested removing the redundant localRouter variable and using the group-level router variable directly to maintain consistency with the existing test suite.
Comment on lines
+911
to
+912
| late GoRouter localRouter; | ||
| localRouter = GoRouter( |
There was a problem hiding this comment.
Comment on lines
+944
to
+949
| router = localRouter; | ||
|
|
||
| await tester.pumpWidget(MaterialApp.router(routerConfig: localRouter)); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(localRouter.state.uri.path, equals('/login')); |
There was a problem hiding this comment.
Update these lines to use the group-level router variable directly and remove the redundant localRouter assignment to maintain consistency with the rest of the test suite.
Suggested change
| router = localRouter; | |
| await tester.pumpWidget(MaterialApp.router(routerConfig: localRouter)); | |
| await tester.pumpAndSettle(); | |
| expect(localRouter.state.uri.path, equals('/login')); | |
| await tester.pumpWidget(MaterialApp.router(routerConfig: router)); | |
| await tester.pumpAndSettle(); | |
| expect(router.state.uri.path, equals('/login')); |
…uter Address gemini-code-assist feedback on flutter#11725: remove the redundant `localRouter` variable and use the existing group-level `router` directly, matching the pattern used in every other test in this file.
11 tasks
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
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.
Adds a regression test for flutter/flutter#179370. The user-visible symptom — a synchronous `onEnter` returning `Block.then(() => router.go(...))` failing to redirect on the initial route — was fixed in go_router 17.2.0 by #11136 (which replaced `await Future.sync(callback)` with `scheduleMicrotask(() async { await callback(); })` in `_OnEnterHandler.handleTopOnEnter` to break the re-entrant parse cycle), but the linked issue is still open and there is no test that fails when that change is reverted.
The new test in `packages/go_router/test/on_enter_test.dart` mirrors the exact predicate the reporter used (`current.uri.path == '/dashboard'` on the initial parse, where `currentState` equals `nextState` because the router has no committed configuration yet) and asserts that the sync `Block.then(router.go('/login'))` redirect resolves to `/login`.
I verified the test is a real regression test by temporarily reverting parser.dart to the pre-#11136 implementation (`await Future.sync(callback)`) and confirming this test fails with that change in place; with the current implementation it passes alongside the rest of `on_enter_test.dart` (32/32, all locally with stable Flutter SDK 3.41.7).
Fixes flutter/flutter#179370.
Pre-Review Checklist
Test-only change, so no version bump / CHANGELOG entry per the published-package exemption. Happy to bump if the reviewer prefers.