driver nnbd by dnfield · Pull Request #74856 · flutter/flutter

dnfield · 2021-01-27T23:54:16Z

NNBD migration for flutter_driver.

I've spot checked this on one or two tests from the devicelab locally. If it goes red on devicelab we can revert.

I'll also see if I can get a FROB run, but this will require a g3fix to enable null safety for the package internally.

goderbauer · 2021-01-27T23:59:47Z

 // found in the LICENSE file.

-// @dart = 2.8
+


remove extra lines?

Here and in some of the other files as well below.

Fixing that.

goderbauer · 2021-01-28T00:02:11Z

  ///
  ///  * [getWidgetDiagnostics], which gets the [DiagnosticsNode] of a [Widget].
-  Future<Map<String, Object>> getRenderObjectDiagnostics(
+  Future<Map<String, dynamic>> getRenderObjectDiagnostics(


Why not Object anymore?

Here and below.

Over aggressive find and replace. I tried making more things not dynamic and they weren't ready to change. Changing back.

goderbauer · 2021-01-28T00:04:00Z

  /// Creates a SceneDisplayLagSummarizer given the timeline events.
  SceneDisplayLagSummarizer(this.sceneDisplayLagEvents) {
-    for (final TimelineEvent event in sceneDisplayLagEvents) {
+    for (final TimelineEvent event in sceneDisplayLagEvents!) {


Why's the final field sceneDisplayLagEvents nullable in the first place then? Wouldn't this always crash when you instantiate SceneDisplayLagSummarizer(null)?

Yes, updating this. I think we just were missing an assert there.. Making it non-null.

goderbauer · 2021-01-28T00:06:19Z

  /// This is parsed from "traceEvents" data within [json] and sorted by
  /// timestamp. Anything without a valid timestamp is put in the beginning.
-  final List<TimelineEvent> events;
+  final List<TimelineEvent>? events;


It's surprising that this can be null. Can we document that? Or should it always be an (sometimes empty) list?

Hmm. I'm pretty sure the method that parses this was written in an overly cautious way. I was going to not change it, but I'm realizing now that it can only return null if another thing thatisn't allowed to be null (json) was null. I'm going to change it.

I was wrong.

This can be null if the trace events are null in the JSON. I'll document it for now.

I think changing it to an empty list would be fine, but would like to do that separately from this migration in case anything depends on this behavior.

dnfield · 2021-01-28T01:19:49Z

I didn't update the packages correctly - we'll need to get an updated version of dwds that allows for quiver, or see if we can drop quiver.

Since the tool depends on quiver and dwds, it might be worth doing anyway to help unblock the tool.

dnfield · 2021-01-28T01:35:53Z

ok, it looks like we can use fake_async, which is migrated and isn't conflicting in this repo.

goderbauer

LGTM

goderbauer · 2021-01-28T01:42:32Z

 // found in the LICENSE file.

-// @dart = 2.8
+


Looks like it's still not fixed here :)

goderbauer · 2021-01-28T01:43:04Z

 // found in the LICENSE file.

-// @dart = 2.8
+


remove extra blank line

goderbauer · 2021-01-28T01:43:19Z

 // found in the LICENSE file.

-// @dart = 2.8
+


fluttergithubbot · 2021-01-28T03:44:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Windows tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot · 2021-01-28T05:34:06Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Windows tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

dnfield · 2021-01-28T05:51:01Z

Windows flaked twice on me for things I'm pretty sure are unrelated to this patch (native crashes in Dart). Merged up to head and trying again...

flar · 2021-02-01T19:57:25Z

        args: serialized,
-      ).then<Map<String, dynamic>>((vms.Response value) => value.json);
-      response = await _warnIfSlow<Map<String, dynamic>>(
+      ).then<Map<String, dynamic>>((vms.Response value) => value.json!);


We are seeing stack traces in our build failures that come from the _warnIfSlow method following this when it evaluates the future. The error is that a Map<String, dynamic> can't be null, so I believe that ! operator on this line may be encountering a null value here and failing the NNBD assertion.

jonahwilliams · 2021-04-12T23:07:28Z

We're not going to consider cherry picks to stable and beta for null safe driver or integration test. We evaluated this when creating the release branch for flutter 2 and concluded that it was too risky. Null safe driver and integration test will be available in the next beta/stable release

dnfield added 2 commits January 27, 2021 15:45

driver nnbd

0a8b9f8

late

b2faf81

dnfield requested review from goderbauer, jonahwilliams and yjbanov January 27, 2021 23:54

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. labels Jan 27, 2021

google-cla Bot added the cla: yes label Jan 27, 2021

goderbauer reviewed Jan 28, 2021

View reviewed changes

dnfield added 2 commits January 27, 2021 17:40

review and dep update

0ce0281

Merge remote-tracking branch 'upstream/master' into nnbd_fuchsia

23ad782

goderbauer approved these changes Jan 28, 2021

View reviewed changes

more whitespace

46f48e5

dnfield added the waiting for tree to go green label Jan 28, 2021

fluttergithubbot removed the waiting for tree to go green label Jan 28, 2021

dnfield added the waiting for tree to go green label Jan 28, 2021

fluttergithubbot removed the waiting for tree to go green label Jan 28, 2021

Merge remote-tracking branch 'upstream/master' into nnbd_fuchsia

2b90b2d

dnfield merged commit 3ece9c6 into flutter:master Jan 28, 2021

flar reviewed Feb 1, 2021

View reviewed changes

darshankawar mentioned this pull request Mar 15, 2021

Update flutter_driver webdriver dependency to null safety version #78084

Closed

jonahwilliams mentioned this pull request Apr 12, 2021

flutter_driver should be null-safe in stable Flutter 2. #80293

Closed

Conversation

dnfield commented Jan 27, 2021

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!

dnfield commented Jan 28, 2021

Uh oh!

dnfield commented Jan 28, 2021

Uh oh!

goderbauer left a comment

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!

fluttergithubbot commented Jan 28, 2021

Uh oh!

fluttergithubbot commented Jan 28, 2021

Uh oh!

dnfield commented Jan 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Apr 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants