sdk

sdk copied to clipboard

Found 10 file excluded from sound null safety:

• pkg/frontend_server/bin/frontend_server_starter.dart
• pkg/frontend_server/lib/compute_kernel.dart
• pkg/frontend_server/lib/frontend_server.dart
• pkg/frontend_server/lib/src/javascript_bundle.dart
• pkg/frontend_server/lib/src/strong_components.dart
• pkg/frontend_server/test/frontend_server_flutter_suite.dart
• pkg/frontend_server/test/frontend_server_flutter.dart
• pkg/frontend_server/test/frontend_server_test.dart
• pkg/frontend_server/test/src/javascript_bundle_test.dart
• pkg/frontend_server/test/src/strong_components_test.dart

See https://github.com/dart-lang/sdk/issues/49169 for more context.

https://dart-review.googlesource.com/c/sdk/+/247601 migrates those that are not blocked by package:dev_compiler:

• pkg/frontend_server/lib/compute_kernel.dart
• pkg/frontend_server/lib/src/strong_components.dart
• pkg/frontend_server/test/strong_components_test.dart

Rest is blocked on https://github.com/dart-lang/sdk/issues/46617

I finished the DDC migration and closed https://github.com/dart-lang/sdk/issues/46617. The frontend_server migration should be unblocked now.

https://dart-review.googlesource.com/c/sdk/+/253301 Migrates all but one of the tests which relies heavily on mockito.

cc @srawlins would you possibly be able to take a look at the frontend_server_test.dart file and give some advice? I don't think we want to add build_runner as a dep to this package (and thus the sdk etc).

frontend_server_test.dart uses a fair amount of mockito API. 4 mock classes extend Mock:

_MockedBinaryPrinterFactory

This class is instantiated once, and one stub is created for the newBinaryPrinter. This class can probably just extend Fake instead of Mock, with an @override for newBinaryPrinter.

_MockedBinaryPrinter

This is an instantiated placeholder - no mockito API. Can be replaced with a Fake.

_MockedCompiler

Instantiated a few places, and the test uses when to stub behavior, and verify and verifyInOrder to verify calls. This is more core usage of mockito, and would require more work to replace, if we wanted to remove mockito. I'd rather not speculate, but I think the authors could look at each verify and verifyInOrder to see if it is important to verify these things. If it is, verify can more-or-less be replaced with overridden methods which store the given parameters in fields. verifyInOrder is definitely trickier. Again, I think looking at them case-by-case can lead to good tests w/o mockito.

_MockedIncrementalCompiler

This is instantiated in one test, with a few stubs, but they don't use any argument matchers. So this would be a pretty simple transition to a Fake with @overrides.

FYI, it looks like this is in the critical path for migrating flutter/engine to sound null safety.

looks like my CL for most of it (just not the tests) failed to submit and I didn't realize, I can pick that up today and try and get it landed.

hey @jakemac53 any news?

Gah sorry, got sidetracked and forgot about it. Looking now.

Ok, I took a look but there have been a ton of upstream changes and merging them just really wasn't feasible.

Somebody will have to start from scratch on this one.

Generally, does the CFE team own this package or does the VM team? cc @johnniwinther and @a-siva. We'll want to find an owner to push this over the line from a conversion-to-null-safety perspective.

It looks like the current null safety exclusions are:

bin/frontend_server_starter.dart:1:// @dart = 2.9
lib/frontend_server.dart:5:// @dart = 2.9
lib/src/javascript_bundle.dart:5:// @dart = 2.9
lib/src/resident_frontend_server.dart:5:// @dart = 2.9
lib/starter.dart:5:// @dart = 2.9
test/frontend_server_flutter.dart:5:// @dart = 2.9
test/frontend_server_flutter_suite.dart:5:// @dart = 2.9
test/frontend_server_test.dart:5:// @dart = 2.9
test/src/javascript_bundle_test.dart:5:// @dart = 2.9
test/src/resident_frontend_server_test.dart:5:// @dart = 2.9

(I didn't check to see if some of those are intended from the POV of testing pre-null safety)

• @jensjoha @annagrin For planning on who will tackle the migration.

I'll look into migration the frontend server.

One of the issues that I ran into with migrating the frontend server tests was the heavy use of mockito package, I am working on a CL to replace the use of mockito in these tests to make it possible to then migrate these files.

The only file remaining is test/frontend_server_test.dart. @a-siva you are working on this, right?

Yes

Thanks for migrating the tests @jakemac53 !

First step of removing mockito dependency from the tests is here https://dart-review.googlesource.com/c/sdk/+/260001/5

Second step of making the file null safe https://dart-review.googlesource.com/c/sdk/+/260744