Skip to content

Sound null safety for framework and flutter_test #68642

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.

Sign up for GitHub

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

Jump to bottom
Merged
goderbauer merged 15 commits into from
Oct 21, 2020
Merged

Sound null safety for framework and flutter_test #68642

goderbauer merged 15 commits into from
Oct 21, 2020

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Oct 20, 2020 •
edited
Loading

This fixes all issues expect the test failures from test/widgets/widget_inspector_test.dart, which are resolved by @jacob314 in #68661.

Fixes #68562

@flutter-dashboard flutter-dashboard bot added d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 20, 2020
@google-cla google-cla bot added the cla: yes label Oct 20, 2020
jonahwilliams
jonahwilliams reviewed Oct 20, 2020
View reviewed changes
jonahwilliams
jonahwilliams previously approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides layer example LGTM

@goderbauer goderbauer changed the title Null safety sound mode fixes - part 1 Null safety sound mode fixes Oct 20, 2020
@goderbauer goderbauer dismissed jonahwilliams’s stale review October 20, 2020 23:25

Added massive changes.

@shihaohong shihaohong self-requested a review October 20, 2020 23:31
@goderbauer goderbauer mentioned this pull request Oct 20, 2020
Closed
jonahwilliams
jonahwilliams approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if LG to CI

@shihaohong
Copy link
Contributor

shihaohong commented Oct 20, 2020

CI is still sad: https://cirrus-ci.com/task/5469809754243072

@goderbauer
Copy link
Member Author

goderbauer commented Oct 21, 2020

The failing widget_inspector_test.dart are blocked on #68661.

jonahwilliams
jonahwilliams approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 21, 2020

The last few widget inspector failures need:

diff --git a/packages/flutter_test/lib/src/_matchers_io.dart b/packages/flutter_test/lib/src/_matchers_io.dart
index 9c1f02244d..23c7a2b1a2 100644
--- a/packages/flutter_test/lib/src/_matchers_io.dart
+++ b/packages/flutter_test/lib/src/_matchers_io.dart
@@ -52,6 +52,8 @@ class MatchesGoldenFile extends AsyncMatcher {
     Future<ui.Image> imageFuture;
     if (item is Future<ui.Image>) {
       imageFuture = item;
+    } else if (item is Future<ui.Image?>) {
+      imageFuture = item.then((ui.Image? image) => image!);
     } else if (item is ui.Image) {
       imageFuture = Future<ui.Image>.value(item);
     } else {

Additionally you should only pass --sound-null-safety to the framework and flutter_test tests.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 21, 2020

That works too :)

@goderbauer goderbauer changed the title Null safety sound mode fixes Sound null safety for framework and flutter_test Oct 21, 2020
@flutter-dashboard flutter-dashboard bot added the a: tests "flutter test", flutter_test, or one of our tests label Oct 21, 2020
@shihaohong
Copy link
Contributor

shihaohong commented Oct 21, 2020 •
edited
Loading

The failing internal tests are legit, but we can fix the code there there after merging this PR -- Some user code expects the returned value to be a non-nullable type for the result of Navigator.of(context)!.push().

shihaohong
shihaohong reviewed Oct 21, 2020
View reviewed changes
Copy link
Contributor

@shihaohong shihaohong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null-safety code changes themselves LGTM, just some questions about example/layers since I'm less familiar and might've missed some context.

dev/bots/test.dart
await _runFlutterTest(path.join(flutterRoot, 'dev', 'tools', 'vitool'), tableData: bigqueryApi?.tabledata);
await _runFlutterTest(path.join(flutterRoot, 'examples', 'hello_world'), tableData: bigqueryApi?.tabledata);
await _runFlutterTest(path.join(flutterRoot, 'examples', 'layers'), tableData: bigqueryApi?.tabledata);
await _runFlutterTest(path.join(flutterRoot, 'examples', 'layers'), tableData: bigqueryApi?.tabledata, options: mixedModeNullSafetyOptions);
Copy link
Contributor

@shihaohong shihaohong Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might've missed the context, but what about the layer example that's giving it mixedModeNullSafetyOptions vs the rest of the tests?

Copy link
Member Author

@goderbauer goderbauer Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files from this example are imported from the framework tests, so I've only migrated those files. The remaining files are still unmigrated, so the tests for the examples themselves need to run in mixed mode.

examples/layers/rendering/custom_coordinate_systems.dart
// found in the LICENSE file.

// @dart = 2.8
// @dart = 2.10
Copy link
Contributor

@shihaohong shihaohong Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this turn on null-safety, but in mixed mode for examples/layers?

Copy link
Member Author

@goderbauer goderbauer Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows us to use null safety syntax in this file (i.e. the ! and ?).

packages/flutter/test/material/app_test.dart

testWidgets('Return value from pop is correct', (WidgetTester tester) async {
late Future<Object> result;
late Future<Object?> result;
Copy link
Contributor

@shihaohong shihaohong Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal test failure is similar to what caused this test to fail

Copy link
Member Author

@goderbauer goderbauer Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's slightly different, fix is in cl/338282385.

@goderbauer
Copy link
Member Author

goderbauer commented Oct 21, 2020

Submitting, google3 is fixed with cl/338282385.

@goderbauer goderbauer merged commit 42f3709 into flutter:master Oct 21, 2020
@goderbauer goderbauer deleted the soundmode2 branch October 21, 2020 17:13
This was referenced Oct 21, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@shihaohong shihaohong shihaohong left review comments

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Additional type errors surfaced by enabling --sound-null-safety on framework unit tests

3 participants

@goderbauer @shihaohong @jonahwilliams