Provide test API for accessibility announcements #109661

nbayati · 2022-08-17T04:47:16Z

Provide testing API for the accessibility announcements made by the Flutter framework. Currently the consumers of the code have to create a mock message handler and match the exact String value of the announcement which is a Map. So adding any new features to the announcements (like: #107568) would break the existing tests. This PR addresses the issue by providing a testing API which takes care of setting up a mock handler and returns an object for easy comparison.

Fixes #108495

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

packages/flutter_test/lib/src/binding.dart

yjbanov · 2022-08-18T17:42:30Z

packages/flutter_test/lib/src/binding.dart

@@ -106,6 +111,14 @@ mixin TestDefaultBinaryMessengerBinding on BindingBase, ServicesBinding {
  }
 }

+/// An accessibility announcement.
+class Announcement {


Let's rename this to CapturedAccessibilityAnnouncement. Otherwise it sounds too much like a production class name. Also, "announcement" does not by itself imply accessibility.

The only potential issue would be lines getting longer than 80 character. I could always break lines similar to what dart formatter would do though.

packages/flutter_test/lib/src/binding.dart

yjbanov · 2022-08-18T17:42:30Z

packages/flutter_test/lib/src/binding.dart

+  /// Returns a list announcements made by the Flutter framework.
+  List<Announcement>? takeAnnouncements() {
+    assert(inTest);
+    return _announcements;


I think the semantics of "take" means "remove the returned value from the captured list".

Yeah you/re right. I've changed the code to clear out the list afterwards. I don't think we'd need a peak/get function for list. What do you think?

packages/flutter_test/lib/src/binding.dart

yjbanov · 2022-08-18T17:42:30Z

packages/flutter_test/lib/src/binding.dart

+  }
+
+  /// Returns the most recent announcement made by the Flutter framework.
+  Announcement? getLastAnnouncement(){


If this is meant to return the last announcement without removing it, I'd call it peekLastAnnouncement rather than get. But I think perhaps we want take here as well, no?

So take would remove the announcement from the list? In that case if the user calls takeLastAnnouncement twice, the second time what they'll get is not actually the last announcement. I'm not sure if this can cause confusion or if it's a common practice.

Ah, good point. In that case maybe it should be called just takeAnnouncement.

If I remember correctly, we decided that we don't need both takeAnnouncement and takeAnnouncements. So I just changed the name of the function to peekLastAnnouncement without changing.

Based on the other comments, I'm removing this function all together because it's easy to access the last announcement through takeAnnouncements() and we don't really need a separate function.

yjbanov · 2022-08-18T17:42:30Z

packages/flutter_test/lib/src/binding.dart

@@ -700,13 +727,40 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
    // The LiveTestWidgetsFlutterBinding overrides this to report the exception to the console.
  }

+  Future<dynamic> _handleMessage(dynamic mockMessage) async {
+    final Map<dynamic, dynamic> data =
+        (mockMessage as Map<dynamic, dynamic>)['data'] as Map<dynamic, dynamic>;


I don't remember, are the keys in the map really dynamic? I recall them all being Strings.

I tried both
final Map<String, dynamic> data = (mockMessage as Map<dynamic, dynamic>)['data'] as Map<String, dynamic>;
and
final Map<String, dynamic> data = (mockMessage as Map<String, dynamic>)['data'] as Map<String, dynamic>;

In both cases I get a casting error:
the following _CastError was thrown running a test: type '_InternalLinkedHashMap<Object?, Object?>' is not a subtype of type 'Map<String, dynamic>' in type cast

Is there a better way to cast the mockMessage and get the values inside data?

you can use .cast<K, V> to cast to a certain type https://api.flutter.dev/flutter/dart-core/Map/cast.html

Oh, interesting. In that case it's fine to keep it dynamic. @chunhtai's suggestion also works.

Both mockMessage and mockMessage['data'] are dynamic. It seems like cast<K,V>() is defined for Map and there isn't a cast function defined on dynamic type.

@nbayati you can call any method on dynamic. So you can do mockMessage.cast<String, dynamic>() and it should work as long as mockMessage actually has that method.

packages/flutter_test/lib/src/binding.dart

chunhtai · 2022-08-22T16:45:22Z

packages/flutter_test/lib/src/binding.dart

@@ -700,13 +727,40 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
    // The LiveTestWidgetsFlutterBinding overrides this to report the exception to the console.
  }

+  Future<dynamic> _handleMessage(dynamic mockMessage) async {
+    final Map<dynamic, dynamic> data =
+        (mockMessage as Map<dynamic, dynamic>)['data'] as Map<dynamic, dynamic>;


you can use .cast<K, V> to cast to a certain type https://api.flutter.dev/flutter/dart-core/Map/cast.html

packages/flutter_test/lib/src/binding.dart

packages/flutter_test/test/widget_tester_test.dart

yjbanov · 2022-08-22T22:40:33Z

packages/flutter_test/test/widget_tester_test.dart

+      expect(TextDirection.rtl, equals(third.textDirection));
+      expect(Assertiveness.polite, equals(third.assertiveness));
+
+      final List<CapturedAccessibilityAnnouncement>? emptyList = tester.takeAnnouncements();


Good call to check that the list was emptied out!

yjbanov · 2022-08-22T22:40:33Z

packages/flutter_test/test/widget_tester_test.dart

+      expect(emptyList, isNull);
+    });
+
+    test('New test API is not breaking existing tests', () async {


We do plan to break existing tests in the future by always specifying assertiveness in the protocol, right? So I'm not sure if this test provides real protection 🤔

My initial thought was to make sure that the current way of testing still works in general. Meaning that if someone wants to setup their own message handler, they're still able to do so. In another word, the new API doesn't overwrite what the user has specified as the message handler.
You had a good point about the future change though. So I've removed the announcements that didn't have assertiveness specified, since having only one would still accomplish what I had in mind.
Let me know if you still think we don't need this test.

packages/flutter_test/test/widget_tester_test.dart

packages/flutter_test/lib/src/binding.dart

goderbauer · 2022-09-13T22:12:25Z

(triage): @nbayati do you still have plans to follow up on the feedback given above?

nbayati · 2022-09-14T04:00:53Z

(triage): @nbayati do you still have plans to follow up on the feedback given above?

Yes. I have been focusing on another high priority PR, but this is still on my list and I should be able to get back to it pretty soon.

mdebbar

Overall, the PR looks good to me, but I'll defer to @chunhtai and @yjbanov for final approval since they are more familiar with this part of the codebase.

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

@@ -62,6 +62,11 @@ enum EnginePhase {
  sendSemanticsUpdate,
 }

+/// Signature of callbacks to intercept messages on a given channel.


nit: "...callbacks used to intercept..."

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

@@ -62,6 +62,11 @@ enum EnginePhase {
  sendSemanticsUpdate,
 }

+/// Signature of callbacks to intercept messages on a given channel.
+///
+/// see [TestDefaultBinaryMessenger.setMockDecodedMessageHandler] for more details.


nit: s/see/See/

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

@@ -106,6 +111,32 @@ mixin TestDefaultBinaryMessengerBinding on BindingBase, ServicesBinding {
  }
 }

+/// An accessibility announcement made by the Flutter framework.


nit: this class does not represent the announcement itself, but the data captured in the test. I'd describe it as "Accessibility announcement data passed to [SemanticsService.announce] captured in a test."

Thanks for the suggestion. I wasn't sure how to word this one.

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

@@ -106,6 +111,32 @@ mixin TestDefaultBinaryMessengerBinding on BindingBase, ServicesBinding {
  }
 }

+/// An accessibility announcement made by the Flutter framework.
+///
+/// This class is mostly used by the testing api to store the announcements


nit: s/mostly/intended to be/
nit: s/api/API/

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

+/// An accessibility announcement made by the Flutter framework.
+///
+/// This class is mostly used by the testing api to store the announcements
+/// in a user-friendly structure so that the code consumers can check the details


nit: hopefully all our API is user-friendly 😄 Instead, I'd just say "in a structured form"
nit: "code consumer" sounds confusing. Maybe say "so that tests can verify announcement details"?

Thanks for the suggestions, Done!

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

+    return announcements;
+  }
+
+  /// Returns the most recent announcement made by the Flutter framework.


The docs should also explain what it means when this method returns null

Changed the function to return an empty list in that case. Do you still have concerns with the documentation?

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/lib/src/binding.dart

  Future<void> _runTest(
    Future<void> Function() testBody,
    VoidCallback invariantTester,
    String description,
  ) {
    assert(description != null);
    assert(inTest);
+
+    if (TestDefaultBinaryMessengerBinding.instance!.defaultBinaryMessenger


Consider leaving a comment explaining what this does.

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/test/widget_tester_test.dart

@@ -821,6 +822,78 @@ void main() {
      binding.postTest();
    });
  });
+
+  group('Accessibility announcements testing API', () {


We should also test that clean-up logic from postTest works. Since the clean-up takes place as part of testWidgets after the test code is done, it can be done in tearDown, for example:

tearDown(() { expect(checkMockMessageHandler(SystemChannels.accessibility.name, null), isTrue); });

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/test/widget_tester_test.dart

@@ -821,6 +822,78 @@ void main() {
      binding.postTest();
    });
  });
+
+  group('Accessibility announcements testing API', () {
+    testWidgets('Returns last announcement', (WidgetTester tester) async {


Similarly, as a sanity check, we could verify that the mock handler is installed, for example:

expect(checkMockMessageHandler(SystemChannels.accessibility.name, null), isFalse);

Good point! Added the sanity check to the test.

yjbanov · 2022-10-17T17:48:50Z

packages/flutter_test/test/widget_tester_test.dart

+            },
+      ]));
+
+      //remove the handler


nit: // Remove...

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/binding.dart

+/// Signature of callbacks to intercept messages on a given channel.
+///
+/// see [TestDefaultBinaryMessenger.setMockDecodedMessageHandler] for more details.
+typedef _MockMessageHandler = Future<dynamic> Function(dynamic);


Please avoid dynamic. It turns off dart's type system. If you don't have a more specific type, Object? should work.

Thanks for the explanation, done!

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/binding.dart

+///
+/// See [TestWidgetsFlutterBinding.takeAnnouncements].
+class CapturedAccessibilityAnnouncement {
+


nit: remove blank line

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/binding.dart

@@ -700,13 +751,43 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
    // The LiveTestWidgetsFlutterBinding overrides this to report the exception to the console.
  }

+  Future<dynamic> _handleMessage(dynamic mockMessage) async {


Future<void> as return type and Object? mockMessage as argument?

Good point! Done!

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/binding.dart

@@ -700,13 +751,43 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
    // The LiveTestWidgetsFlutterBinding overrides this to report the exception to the console.
  }

+  Future<dynamic> _handleMessage(dynamic mockMessage) async {
+    final Map<dynamic, dynamic> message = mockMessage as Map<dynamic, dynamic>;


Here and elsewhere: Object? instead of dynamic

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/binding.dart

+      final CapturedAccessibilityAnnouncement announcement;
+
+      announcement = CapturedAccessibilityAnnouncement(
+          dataMessage, textDirection,
+          assertiveness: assertiveness);


combine this?

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/binding.dart

+
+  /// Returns the most recent announcement made by the Flutter framework.
+  CapturedAccessibilityAnnouncement? peekLastAnnouncement() {
+    return _announcements?.last;


should this have the inTest assert?

Removed this function based on the other comments.

goderbauer · 2022-10-17T18:43:48Z

packages/flutter_test/lib/src/widget_tester.dart

+  /// Returns the most recent announcement made by the Flutter framework.
+  ///
+  /// See [TestWidgetsFlutterBinding.peekLastAnnouncement] for details.
+  CapturedAccessibilityAnnouncement? peekLastAnnouncement() {


Do we need both of these APIs? Wouldn't it be enough to just have the takeAnnouncements one? If we go with both, the API docs should link to the other one in a "see also" section.

I agree. Removed peekLastAnnouncement() since having the takeAnnouncements() seems enough.

yjbanov

LGTM but since this is a framework-side change that affects more than just the web, I think we should also look for an approval from either @chunhtai or @goderbauer.

yjbanov · 2022-10-19T16:44:01Z

packages/flutter_test/lib/src/binding.dart

+///
+/// This class is intended to be used by the testing API to store the announcements
+/// in a structured form so that tests can verify announcement details.
+/// the fields of this class correspond to parameters of the [SemanticsService.announce] method.


s/the/The/

Separate paragraphs by one empty line. Or if this was meant to be a continuation of the previous paragraph, there's still space on the previous line to begin this sentence.

yjbanov · 2022-10-19T16:44:01Z

packages/flutter_test/lib/src/binding.dart

+  /// The accessibility message announced by the framework.
+  final String message;
+
+  /// The direction in which text flows.


nit: I'd add "the text of the [message] flows"

yjbanov · 2022-10-19T16:44:01Z

packages/flutter_test/lib/src/binding.dart

@@ -700,13 +747,41 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
    // The LiveTestWidgetsFlutterBinding overrides this to report the exception to the console.
  }

+  Future<void> _handleMessage(Object? mockMessage) async {


nit: _handleMessage sounds too generic. This is handling announcement messages specifically. Maybe call it _handleAnnouncementMessage?

yjbanov · 2022-10-19T16:44:01Z

packages/flutter_test/lib/src/widget_tester.dart

+  /// Returns a list of all the accessibility announcements made by the Flutter
+  /// framework since the last time this function was called.
+  ///
+  /// See [TestWidgetsFlutterBinding.takeAnnouncements] for details.


If the documentation can be identical, instead of pointing to another place, you can use dartdoc templates. Here's an example:

Create a template:

flutter/packages/flutter/lib/src/cupertino/nav_bar.dart

Line 272 in 9bdd50a

/// {@template flutter.cupertino.CupertinoNavigationBar.leading}

Copy it to another place using a macro:

flutter/packages/flutter/lib/src/cupertino/nav_bar.dart

Line 631 in 9bdd50a

/// {@macro flutter.cupertino.CupertinoNavigationBar.leading}

Cool feature! Thanks for pointing it out.

chunhtai

LGTM once all the comments from other reviewers are addressed

goderbauer

LGTM

goderbauer · 2022-10-26T16:07:19Z

packages/flutter_test/lib/src/binding.dart

@@ -611,6 +642,24 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
  late StackTraceDemangler _oldStackTraceDemangler;
  FlutterErrorDetails? _pendingExceptionDetails;

+  _MockMessageHandler? _announcementHandler;


Why do we need this variable? Could you just use _handleAnnouncementMessage directly wherever it is used?

goderbauer · 2022-10-26T16:07:19Z

packages/flutter_test/test/widget_tester_test.dart

+      expect(third.assertiveness, Assertiveness.polite);
+
+      final List<CapturedAccessibilityAnnouncement> emptyList = tester.takeAnnouncements();
+      expect(emptyList, <CapturedAccessibilityAnnouncement>[]);


nit: There is the isEmpty matcher, which I think would make this slightly more readable:

Suggested change

expect(emptyList, <CapturedAccessibilityAnnouncement>[]);

expect(emptyList, isEmpty);

Provide test API for accessibility announcements

3022976

flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests. framework flutter/packages/flutter repository. See also f: labels. labels Aug 17, 2022

nbayati requested a review from yjbanov Aug 17, 2022

yjbanov reviewed Aug 18, 2022

View changes

nbayati added 4 commits Aug 19, 2022

Merge branch 'main' into announce-test-api

5b67737

Change class name and some minor fixes

3194464

Merge branch 'main' into announce-test-api

c92620f

Add tests and minor fixes

41f53dc

nbayati requested review from chunhtai and yjbanov Aug 20, 2022

chunhtai reviewed Aug 22, 2022

View changes

yjbanov reviewed Aug 22, 2022

View changes

nbayati added 3 commits Oct 4, 2022

Merge branch 'main' into announce-test-api

783c98d

Merge branch 'main' into announce-test-api

cf099d9

minor fixes

91e8548

nbayati requested review from mdebbar, chunhtai and yjbanov Oct 6, 2022

mdebbar reviewed Oct 7, 2022

View changes

nbayati removed request for yjbanov and chunhtai Oct 13, 2022

nbayati added 3 commits Oct 13, 2022

Merge branch 'main' into announce-test-api

5090efe

add some documentation

8ca26cb

set the handler only if currently there is nonee

c04be0a

nbayati marked this pull request as ready for review Oct 14, 2022

nbayati added 2 commits Oct 15, 2022

fix a bug with storing message handler

e4f7e33

remove the handler after test runs

493a742

nbayati requested review from yjbanov and chunhtai Oct 15, 2022

yjbanov reviewed Oct 17, 2022

View changes

goderbauer reviewed Oct 17, 2022

View changes

nbayati added 3 commits Oct 17, 2022

Merge branch 'main' into announce-test-api

45ca1c9

Merge branch 'main' into announce-test-api

9ac3c54

code enhancements

eedef94

nbayati requested review from yjbanov, mdebbar and goderbauer and removed request for mdebbar Oct 18, 2022

yjbanov approved these changes Oct 19, 2022

View changes

chunhtai approved these changes Oct 19, 2022

View changes

Documentation enhancements

eb9c0b1

goderbauer approved these changes Oct 26, 2022

View changes

nbayati added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 26, 2022

auto-submit bot merged commit 235a325 into flutter:master Oct 26, 2022
64 checks passed

	expect(emptyList, <CapturedAccessibilityAnnouncement>[]);
	expect(emptyList, isEmpty);

Provide test API for accessibility announcements #109661

Provide test API for accessibility announcements #109661

Conversation

nbayati commented Aug 17, 2022 • edited

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbayati Oct 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer commented Sep 13, 2022

nbayati commented Sep 14, 2022

mdebbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbayati commented Aug 17, 2022 •

edited

nbayati Oct 6, 2022 •

edited