Remove Material import from navigator_on_did_remove_page_test#184776
Remove Material import from navigator_on_did_remove_page_test#184776xfce0 wants to merge 0 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes a test file from the cross-import checker and updates the navigator test to use a custom _TestPage implementation instead of MaterialPage. The review feedback suggests adding documentation to the new helper class and ensuring consistent animation durations in the PageRouteBuilder for test performance.
| class _TestPage extends Page<void> { | ||
| const _TestPage({required super.key, required this.child}); | ||
|
|
||
| final Widget child; |
There was a problem hiding this comment.
According to the Flutter style guide, all members (including private ones) should have public-quality documentation using ///. Adding documentation helps explain the purpose of this test helper class and its fields.
/// A [Page] implementation used for testing the [Navigator].
class _TestPage extends Page<void> {
/// Creates a [_TestPage].
const _TestPage({required super.key, required this.child});
/// The widget to display as the content of the page.
final Widget child;References
- Use
///for public-quality documentation, even on private members. (link)
| PageRouteBuilder<void>( | ||
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | ||
| return const Text('new page'); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
For consistency with _TestPage and to ensure the test remains fast and deterministic, consider setting transitionDuration and reverseTransitionDuration to Duration.zero here as well. This avoids unnecessary animation delays during the test execution.
| PageRouteBuilder<void>( | |
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | |
| return const Text('new page'); | |
| }, | |
| ), | |
| PageRouteBuilder<void>( | |
| transitionDuration: Duration.zero, | |
| reverseTransitionDuration: Duration.zero, | |
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | |
| return const Text('new page'); | |
| }, | |
| ), |
There was a problem hiding this comment.
I'm not sure whether this has an actual effect on test execution time, but probable worth trying it.
justinmc
left a comment
There was a problem hiding this comment.
Thanks for the PR! Take a look at using the existing TestPage here. I think it was added recently, my fault for the delay in reviewing this PR.
Also heads up that there is an analyzer failure.
| import 'package:flutter/widgets.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
|
|
||
| class _TestPage extends Page<void> { |
There was a problem hiding this comment.
Could this use this shared utility?
There was a problem hiding this comment.
Done — replaced the local _TestPage with the shared TestPage<void> from test_page_tester.dart.
| PageRouteBuilder<void>( | ||
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | ||
| return const Text('new page'); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
I'm not sure whether this has an actual effect on test execution time, but probable worth trying it.
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
0179e25 to
2150dfd
Compare
Part of #177414.
Replaces
MaterialPagewith a local_TestPagehelper andMaterialPageRoutewithPageRouteBuilder, removing the unnecessary dependency onpackage:flutter/material.dartfrom a widgets-layer test.Also removes the file from the
knownWidgetsCrossImportsallowlist indev/bots/check_tests_cross_imports.dart.Pre-launch Checklist
///).