-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Make last announced routes nullable. #144031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| expect(previousOfFirst, isNull); | ||
| expect(nextOfFirst, secondRoute); | ||
| expect(popNextOfFirst, isA<NotAnnounced>()); | ||
| expect(popNextOfFirst, isNull); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the test made a distinction between null and isA<NotAnnounced> (e.g. compare line 2342 with line 2344). Now, the test cannot differentiate between null and the null that's replacing isA<NotAnnounced> anymore. What test coverage (if any) are we losing because of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, to verify that the changes in navigator.dart don't introduce any unintended behavior changes: Does this test still pass in its unmodified version (using the test-defined NotAnnounced class) with the modifications to navigator.dart applied? There shouldn't really be a reason to remove the NotAnnounced class from this test, the test could just dispose the instances at the end (or better: the test could create one instance only and dispose that at the end).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did that experiment real quick locally and the test is failing, see below. So, the changes in navigator.dart are introducing a change in behavior that the "refactoring" to this test seem to hide.
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: null
Actual: <Instance of 'NotAnnounced'>
When the exception was thrown, this was the stack:
#4 main.<anonymous closure> (file:///Users/goderbauer/dev/flutter/packages/flutter/test/widgets/navigator_test.dart:2342:5)
<asynchronous suspension>
#5 testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:183:15)
<asynchronous suspension>
#6 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1021:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)
This was caught by the test expectation on the following line:
file:///Users/goderbauer/dev/flutter/packages/flutter/test/widgets/navigator_test.dart line 2342
The test description was:
Route announce correctly for first route and last route
════════════════════════════════════════════════════════════════════════════════════════════════════
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done. I had to change one line in the test to make it working. This means, yes, the behavior it changed.
So, #144050 is a suggested way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo... is this changed behavior still correct? Or is this introducing a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely introduced a bug.
I need to be more alerted if tests are updated for a refactoring PR.
Thanks for guidance!
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Contributes to dart-lang/leak_tracker#218