Migrate flutter_test by goderbauer · Pull Request #66663 · flutter/flutter

goderbauer · 2020-09-25T18:38:55Z

Description

Migrates flutter_test to null-safety.

I will migrate the tests of this package in a separate PR.

Related Issues

Fixes #53908.

Tests

I added the following tests:

n/a

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

goderbauer · 2020-09-28T00:19:27Z

Marking this PR as ready for review triggered a whole bunch of new checks. Probably worthwhile to wait and see if all those pass before starting a review.

gspencergoog · 2020-09-28T01:17:46Z

Looks like they passed. Yay! I'll take a look at this tomorrow.

a14n · 2020-09-28T06:25:23Z

packages/flutter_test/lib/src/_binding_io.dart

Suggested change

Future<HttpClientResponse> get done {

return Future<HttpClientResponse>.value(_MockHttpResponse());

}

Future<HttpClientResponse> get done async => _MockHttpResponse();

a14n · 2020-09-28T06:30:36Z

packages/flutter_test/lib/src/accessibility.dart

Suggested change

final RenderBox renderObject = element.renderObject as RenderBox;

final RenderBox renderObject = element.renderObject! as RenderBox;

a14n · 2020-09-28T06:33:30Z

packages/flutter_test/lib/src/accessibility.dart

Perhaps throwing something is better here?

a14n · 2020-09-28T06:35:55Z

packages/flutter_test/lib/src/accessibility.dart

== true could be removed.

a14n · 2020-09-28T06:37:23Z

packages/flutter_test/lib/src/animation_sheet.dart

Suggested change

final RenderRepaintBoundary boundary = boundaryKey.currentContext!.findRenderObject() as RenderRepaintBoundary;

final RenderRepaintBoundary boundary = boundaryKey.currentContext!.findRenderObject()! as RenderRepaintBoundary;

a14n · 2020-09-28T06:49:18Z

packages/flutter_test/lib/src/controller.dart

Suggested change

renderObject = renderObject.parent as RenderObject;

renderObject = renderObject.parent! as RenderObject;

a14n · 2020-09-28T06:51:34Z

packages/flutter_test/lib/src/event_simulation.dart

packages/flutter_test/analysis_options.yaml

packages/flutter/test/cupertino/text_field_test.dart

packages/flutter_test/lib/src/_binding_io.dart

packages/flutter_test/lib/src/accessibility.dart

packages/flutter_test/lib/src/binding.dart

gspencergoog · 2020-09-28T17:18:09Z

packages/flutter_test/lib/src/test_async_utils.dart

Doesn't casting it to Object (as opposed to Object?) imply the !?

Yes, it does. But @a14n is working on a lint that will require you to specifically think about null and add the ! to say that you thought about nullability and this cannot be null here.

OK. When that lint is turned on, I'm sure we'll have a bunch of places to fix.

Yeah, a14n has already been fixing up our code base in prep for that.

packages/flutter_test/lib/src/test_compat.dart

packages/flutter_test/lib/src/window.dart

goderbauer · 2020-09-28T17:45:43Z

@a14n @gspencergoog All comments addressed. Thanks for the review! PTAL.

goderbauer · 2020-09-28T18:21:42Z

While trying this change out in the google codebase I noticed a minor setback: flutter_test does import the material lib, which is not fully migrated yet. Specifically, flutter_test uses the Tooltip and the Card type to implement the find.byTooltip finder and the isInCard matcher.

Since in google internally, a lib can only have null safety turned on when all its imports are also null safe, we will have to migrate material's Tooltip and Card (plus all their imports) first before we can submit this.

/cc @a14n @gspencergoog

Hixie · 2020-09-28T20:08:54Z

packages/flutter_test/analysis_options.yaml

that second bug is closed

Hixie · 2020-09-28T20:21:12Z

packages/flutter_test/lib/src/_matchers_io.dart

should we check if renderObject is non-null? Or is it more or less guaranteed to be?

Added an assert.

Hixie · 2020-09-28T20:21:35Z

packages/flutter_test/lib/src/_matchers_web.dart

Hixie · 2020-09-28T20:21:59Z

packages/flutter_test/lib/src/_matchers_web.dart

similar here; what if someone ends up calling this before layout/paint have run? you can change the phase in a test.

Hixie · 2020-09-28T20:23:01Z

packages/flutter_test/lib/src/accessibility.dart

yikes, do we document / check that this is only used with RenderBoxes?

I added an assert. Currently, it is guaranteed that the renderObject will always be a RenderBox because the element finder above only returns Elements of Text or Editable widgets, which will always have RenderBox elements.

Hixie · 2020-09-28T20:26:08Z

packages/flutter_test/lib/src/binding.dart

docs should mention T

Hixie · 2020-09-28T20:29:09Z

packages/flutter_test/lib/src/controller.dart

nit: switch ((space before(`)

Hixie · 2020-09-28T20:29:37Z

packages/flutter_test/lib/src/event_simulation.dart

fyi @gspencergoog

Yes, I saw this when I did my review, and it simplifies other code here, but doesn't change behavior, so a net win.

Hixie · 2020-09-28T20:35:08Z

packages/flutter_test/lib/src/widget_tester.dart

i wonder how many people ever use this and whether we should just remove it. I don't think I've used it once since implementing it.

(just an idle thought unrelated to this PR obviously)

Hixie

flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Sep 25, 2020

googlebot added the cla: yes label Sep 25, 2020

This comment has been minimized.

Sign in to view

goderbauer force-pushed the flutter_test_mirgation branch from b8aa3f6 to 839f517 Compare September 25, 2020 19:15

This comment has been minimized.

Sign in to view

goderbauer force-pushed the flutter_test_mirgation branch from b33ea19 to 2f0c561 Compare September 25, 2020 23:26

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Sep 28, 2020

goderbauer marked this pull request as ready for review September 28, 2020 00:10

goderbauer requested a review from Piinks as a code owner September 28, 2020 00:10

goderbauer requested a review from gspencergoog September 28, 2020 00:11

goderbauer removed the work in progress; do not review label Sep 28, 2020

goderbauer mentioned this pull request Sep 28, 2020

☂️ Migrate framework tests to null safety #62886

Closed

4 tasks

a14n reviewed Sep 28, 2020

View reviewed changes

goderbauer force-pushed the flutter_test_mirgation branch from 2f0c561 to d19b8ec Compare September 28, 2020 16:53

gspencergoog requested changes Sep 28, 2020

View reviewed changes

gspencergoog approved these changes Sep 28, 2020

View reviewed changes

goderbauer mentioned this pull request Sep 28, 2020

Enable null safety for flutter_test #53908

Closed

12 tasks

Hixie reviewed Sep 28, 2020

View reviewed changes

packages/flutter_test/lib/src/_matchers_web.dart Outdated

Copy link

Contributor

Hixie Sep 28, 2020

Choose a reason for hiding this comment

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

ditto

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 28, 2020

View reviewed changes

packages/flutter_test/lib/src/binding.dart Outdated

Copy link

Contributor

Hixie Sep 28, 2020

Choose a reason for hiding this comment

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

docs should mention T

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 28, 2020

View reviewed changes

packages/flutter_test/lib/src/controller.dart Outdated

Copy link

Contributor

Hixie Sep 28, 2020

Choose a reason for hiding this comment

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

nit: switch ((space before(`)

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 28, 2020

View reviewed changes

Hixie approved these changes Sep 28, 2020

View reviewed changes

goderbauer force-pushed the flutter_test_mirgation branch 3 times, most recently from 4738563 to 71a7d08 Compare September 30, 2020 16:38

goderbauer added 13 commits September 30, 2020 15:52

Migrate flutter_test

adad94c

++

6b5fbe0

++

d3a777c

++

2eb1f67

++

1b16f3b

++

9588b8c

++

642a0d9

++

b04baa7

a14n review

597b818

fixes after merging upstream

f8956b8

greg review

1f764f5

fix imports and make material imports explicit

e3a8ebf

ian review

6316779

goderbauer force-pushed the flutter_test_mirgation branch from 71a7d08 to 6316779 Compare September 30, 2020 23:13

goderbauer added the waiting for tree to go green label Sep 30, 2020

goderbauer merged commit 19e07d2 into flutter:master Oct 1, 2020

goderbauer deleted the flutter_test_mirgation branch October 1, 2020 00:03

-  Future<HttpClientResponse> get done {
-    return Future<HttpClientResponse>.value(_MockHttpResponse());
-  }
+  Future<HttpClientResponse> get done async => _MockHttpResponse();

	final RenderBox renderObject = element.renderObject as RenderBox;
	final RenderBox renderObject = element.renderObject! as RenderBox;

	final RenderRepaintBoundary boundary = boundaryKey.currentContext!.findRenderObject() as RenderRepaintBoundary;
	final RenderRepaintBoundary boundary = boundaryKey.currentContext!.findRenderObject()! as RenderRepaintBoundary;

	renderObject = renderObject.parent as RenderObject;
	renderObject = renderObject.parent! as RenderObject;

Conversation

goderbauer commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

goderbauer commented Sep 28, 2020

Uh oh!

gspencergoog commented Sep 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Sep 28, 2020

Uh oh!

goderbauer commented Sep 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

goderbauer commented Sep 25, 2020 •

edited

Loading