Skip to content

Conversation

@polina-c
Copy link
Contributor

@polina-c polina-c commented Feb 23, 2024

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Feb 23, 2024
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Feb 24, 2024
@polina-c polina-c marked this pull request as ready for review February 24, 2024 19:10
@polina-c polina-c requested a review from jacob314 February 24, 2024 19:12
Comment on lines 2342 to 2344
expect(previousOfFirst, isNull);
expect(nextOfFirst, secondRoute);
expect(popNextOfFirst, isA<NotAnnounced>());
expect(popNextOfFirst, isNull);
Copy link
Member

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?

Copy link
Member

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).

Copy link
Member

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
════════════════════════════════════════════════════════════════════════════════════════════════════

Copy link
Contributor Author

@polina-c polina-c Feb 26, 2024

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.

Copy link
Member

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?

Copy link
Contributor Author

@polina-c polina-c Feb 27, 2024

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!

@polina-c polina-c marked this pull request as draft February 26, 2024 18:24
@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@polina-c polina-c requested a review from goderbauer February 26, 2024 22:18
@polina-c polina-c closed this Feb 27, 2024
@polina-c polina-c deleted the notAnnounced branch February 28, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants