Migrate test data and infra to null safety.

[polina-c]

No description provided.

[kenzieschmoll]

late

[polina-c]

I am confused by this below:

    if (proc != null) {
      debugPrint('Waiting for process to end');
      return proc!.exitCode.timeout(quitTimeout, onTimeout: killGracefully);
    }

proc is expected to become null at some point, but I do not see the code that assignes it to null. Am I missing something?

[kenzieschmoll]

searching for "proc = " I only see where proc is assigned to the result of await Process.start(...). How does the code snippet you pasted relate to marking proc as late? Does it give you an error? Can you remove the if (proc != null)?

[polina-c]

fixed

[kenzieschmoll]

why does this need a nullable cast? _flutterDriverFactory should return a non-nullable value

[bkonyi]

_flutter can probably be marked as a late field instead of being marked nullable (assuming setupEnvironment is always invoked at some point).

[polina-c]

it is assigned to null in tearDownEnvironment

[bkonyi]

Kenzie covered most of the comments I have, but I found a few more places we can remove null checks.

[bkonyi]

vm.isolates will never be null in practice, but it could be empty so you'll need a check to make sure there's an element before calling .first. Also, you can always assume an ID is provided (it's only nullable since tests with mocks like to omit them).

[bkonyi]

Assign vmService to a local so we only do one null check.

[polina-c]

If there were at least two null assersions, it would make sense. There is just one.

[bkonyi]

If you do this:

final service = vmService;
if (service != null) {
  await service.dispose();
}

We only perform one null check instead of two. Anywhere with the pattern:

if (foo != null) {
  foo!.doSomething();
}

should either be updated to:

final localFoo = foo;
if (localFoo != null) {
  localFoo.doSomething();
}

or

foo?.doSomething();

[polina-c]

Hm. What's the point to update

if (foo != null) {
  foo!.doSomething();
}

with

final localFoo = foo;
if (localFoo != null) {
  localFoo.doSomething();
}

?

The second piece seems less readable to me. Is there performance improvement?

[bkonyi]

In theory, yes. The code:

if (foo != null) {
  foo!.doSomething();
}

Effectively desugars to:

if (foo != null) {
  if (foo == null) {
    throw NullAssertionError();
  }
  foo.doSomething();
}

So we end up doing two null checks on foo. I wouldn't expect this to make a huge difference in performance in simple cases like this, but it's possible the compiler might try and do some optimizations when it knows nullable fields are non-null.

I'm not a huge fan of the readability either, but it's what we do throughout the core libraries too. An option to make it slightly better is to take advantage of (abuse?) variable shadowing to re-declare the variable in the scope of the function:

final foo = this.foo;
if (foo != null) {
  // Note: we no longer need a null assertion here since we're using the shadow variable.
  foo.doSomething();
}

I'm still not sure how I feel about this, but it definitely improves readability. For simple cases like this, I think it should be fine.

[kenzieschmoll]

@leafpetersen @munificent do you have an opinion on whether we should recommend this style to users?

[leafpetersen]

@munificent may have more to say here. Obviously, when applicable the foo?.bar() approach is fine (I like it). The style guide already softly recommends the local variable approach ("Consider"). I personally don't mind using a "!" when there's only one occurrence of the variable (even though there is the redundant check), but I'm pretty sure @lrhn yelled at me the last time I did that in a CL, so.... :)

[polina-c]

Thank you for the link. I see there are three usages of '!' in the example. And, yes, the question is what we think about single usage.

There are actually two questions:

1. Do we want to recommend local variable for this case to Dart users?
2. Do we want to document the style as a standard across Dart code libraries?

[leafpetersen]

TBH, my preferred answer to this is that you should use the new pattern feature that @munificent is working on to do the null check and the local variable binding in one step inside of the condition. Unfortunately he hasn't shipped it yet, dunno what the hold up is... :).

So I'll defer to others as to what we want to do in the interim, but I would caution against putting large effort into standardizing/cleaning up around one of the existing solutions given that I expect (hope) that we will be shipping a better solution within a year.

[polina-c]

That's cool! Let's wait then. I am curious how the code would look like.

[bkonyi]

_flutter can probably be marked as a late field instead of being marked nullable (assuming setupEnvironment is always invoked at some point).

[polina-c]

@bkonyi , replying your comments that do not have button 'Reply':

#3873 (comment)
_flutter is null checked, so it cannot be marked as late.

#3873 (comment)
I decided not to check for length before calling first, because this code does not fail for all existing tests. If/when it start failing, the test author will need to figure out what to do in case of empty collection. Makes sense?