-
Notifications
You must be signed in to change notification settings - Fork 9.7k
nnbd for integration_test #3158
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| include: ../../analysis_options.yaml | ||
| analyzer: | ||
| enable-experiment: | ||
| - non-nullable |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,7 +80,7 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| @override | ||
| bool get registerTestTextInput => false; | ||
|
|
||
| Size _surfaceSize; | ||
| Size? _surfaceSize; | ||
|
|
||
| // This flag is used to print warning messages when tracking performance | ||
| // under debug mode. | ||
|
|
@@ -91,7 +91,7 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| /// | ||
| /// Set to null to use the default surface size. | ||
| @override | ||
| Future<void> setSurfaceSize(Size size) { | ||
| Future<void> setSurfaceSize(Size? size) { | ||
| return TestAsyncUtils.guard<void>(() async { | ||
| assert(inTest); | ||
| if (_surfaceSize == size) { | ||
|
|
@@ -123,12 +123,17 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| /// | ||
| /// Returns an instance of the [IntegrationTestWidgetsFlutterBinding], creating and | ||
| /// initializing it if necessary. | ||
| static WidgetsBinding ensureInitialized() { | ||
| static WidgetsBinding ensureInitialized( | ||
| {@visibleForTesting vm.VmService? vmService}) { | ||
| if (WidgetsBinding.instance == null) { | ||
| IntegrationTestWidgetsFlutterBinding(); | ||
| } | ||
| assert(WidgetsBinding.instance is IntegrationTestWidgetsFlutterBinding); | ||
| return WidgetsBinding.instance; | ||
| if (vmService != null) { | ||
| (WidgetsBinding.instance as IntegrationTestWidgetsFlutterBinding) | ||
| ._cachedVmService = vmService; | ||
| } | ||
| return WidgetsBinding.instance!; | ||
| } | ||
|
|
||
| static const MethodChannel _channel = | ||
|
|
@@ -150,9 +155,9 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| /// | ||
| /// The default value is `null`. | ||
| @override | ||
| Map<String, dynamic> get reportData => _reportData; | ||
| Map<String, dynamic> _reportData; | ||
| set reportData(Map<String, dynamic> data) => this._reportData = data; | ||
| Map<String, dynamic>? get reportData => _reportData; | ||
| Map<String, dynamic>? _reportData; | ||
| set reportData(Map<String, dynamic>? data) => this._reportData = data; | ||
|
|
||
| /// Manages callbacks received from driver side and commands send to driver | ||
| /// side. | ||
|
|
@@ -189,7 +194,7 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| Future<void> testBody(), | ||
| VoidCallback invariantTester, { | ||
| String description = '', | ||
| Duration timeout, | ||
| Duration? timeout, | ||
| }) async { | ||
| await super.runTest( | ||
| testBody, | ||
|
|
@@ -200,28 +205,28 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| results[description] ??= _success; | ||
| } | ||
|
|
||
| vm.VmService _vmService; | ||
| vm.VmService? _cachedVmService; | ||
| Future<vm.VmService> get _vmService async { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change related to nnbd?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be split out into a separate patch. It came up because with nnbd I had to think more carefully about whether this property was nullable or not, and when it'd be set, and came to realize it was getting set by a method when another method might want to use it too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, let's keep it here. I think it's enough as long as we add it to the change log/pr description (if something goes wrong it will be easier to see 😅 ) |
||
| if (_cachedVmService == null) { | ||
| final developer.ServiceProtocolInfo info = | ||
| await developer.Service.getInfo(); | ||
| assert(info.serverUri != null); | ||
| _cachedVmService = await vm_io.vmServiceConnectUri( | ||
| 'ws://localhost:${info.serverUri!.port}${info.serverUri!.path}ws', | ||
| ); | ||
| } | ||
| return _cachedVmService!; | ||
| } | ||
|
|
||
| /// Initialize the [vm.VmService] settings for the timeline. | ||
| @visibleForTesting | ||
| Future<void> enableTimeline({ | ||
| List<String> streams = const <String>['all'], | ||
| @visibleForTesting vm.VmService vmService, | ||
| }) async { | ||
| assert(streams != null); | ||
| assert(streams != null); // ignore: unnecessary_null_comparison | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line seems no longer necessary when we have null-safety.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's still useful when not in sound null safety e.g. importing a non nnbd package
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: does it make sense to merge to asserts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nturgut I'm not sure I'm understanding what merge asserts would be in this context...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's very nit :) assert(streams !=null && streams. isNotEmpty, 'Non empty stream is required') |
||
| assert(streams.isNotEmpty); | ||
| if (vmService != null) { | ||
| _vmService = vmService; | ||
| } | ||
| if (_vmService == null) { | ||
| final developer.ServiceProtocolInfo info = | ||
| await developer.Service.getInfo(); | ||
| assert(info.serverUri != null); | ||
| _vmService = await vm_io.vmServiceConnectUri( | ||
| 'ws://localhost:${info.serverUri.port}${info.serverUri.path}ws', | ||
| ); | ||
| } | ||
| await _vmService.setVMTimelineFlags(streams); | ||
| final vm.VmService vmService = await _vmService; | ||
| await vmService.setVMTimelineFlags(streams); | ||
| } | ||
|
|
||
| /// Runs [action] and returns a [vm.Timeline] trace for it. | ||
|
|
@@ -243,16 +248,17 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| bool retainPriorEvents = false, | ||
| }) async { | ||
| await enableTimeline(streams: streams); | ||
| final vm.VmService vmService = await _vmService; | ||
| if (retainPriorEvents) { | ||
| await action(); | ||
| return await _vmService.getVMTimeline(); | ||
| return await vmService.getVMTimeline(); | ||
| } | ||
|
|
||
| await _vmService.clearVMTimeline(); | ||
| final vm.Timestamp startTime = await _vmService.getVMTimelineMicros(); | ||
| await vmService.clearVMTimeline(); | ||
| final vm.Timestamp startTime = await vmService.getVMTimelineMicros(); | ||
| await action(); | ||
| final vm.Timestamp endTime = await _vmService.getVMTimelineMicros(); | ||
| return await _vmService.getVMTimeline( | ||
| final vm.Timestamp endTime = await vmService.getVMTimelineMicros(); | ||
| return await vmService.getVMTimeline( | ||
| timeOriginMicros: startTime.timestamp, | ||
| timeExtentMicros: endTime.timestamp, | ||
| ); | ||
|
|
@@ -285,7 +291,7 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| retainPriorEvents: retainPriorEvents, | ||
| ); | ||
| reportData ??= <String, dynamic>{}; | ||
| reportData[reportKey] = timeline.toJson(); | ||
| reportData![reportKey] = timeline.toJson(); | ||
| } | ||
|
|
||
| /// Watches the [FrameTiming] during `action` and report it to the binding | ||
|
|
@@ -324,6 +330,6 @@ class IntegrationTestWidgetsFlutterBinding extends LiveTestWidgetsFlutterBinding | |
| final FrameTimingSummarizer frameTimes = | ||
| FrameTimingSummarizer(frameTimings); | ||
| reportData ??= <String, dynamic>{}; | ||
| reportData[reportKey] = frameTimes.summary; | ||
| reportData![reportKey] = frameTimes.summary; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,9 @@ typedef ResponseDataCallback = FutureOr<void> Function(Map<String, dynamic>); | |
| Future<void> writeResponseData( | ||
| Map<String, dynamic> data, { | ||
| String testOutputFilename = 'integration_response_data', | ||
| String destinationDirectory, | ||
| String? destinationDirectory, | ||
| }) async { | ||
| assert(testOutputFilename != null); | ||
| assert(testOutputFilename != null); // ignore: unnecessary_null_comparison | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line seems no longer necessary when we have null-safety.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above - these are still helpful when working in mixed/unsound mode. |
||
| destinationDirectory ??= testOutputsDirectory; | ||
| await fs.directory(destinationDirectory).create(recursive: true); | ||
| final File file = fs.file(path.join( | ||
|
|
@@ -65,7 +65,7 @@ Future<void> writeResponseData( | |
| /// ``` | ||
| Future<void> integrationDriver({ | ||
| Duration timeout = const Duration(minutes: 1), | ||
| ResponseDataCallback responseDataCallback = writeResponseData, | ||
| ResponseDataCallback? responseDataCallback = writeResponseData, | ||
| }) async { | ||
| final FlutterDriver driver = await FlutterDriver.connect(); | ||
| final String jsonResult = await driver.requestData(null, timeout: timeout); | ||
|
|
@@ -75,7 +75,7 @@ Future<void> integrationDriver({ | |
| if (response.allTestsPassed) { | ||
| print('All tests passed.'); | ||
| if (responseDataCallback != null) { | ||
| await responseDataCallback(response.data); | ||
| await responseDataCallback(response.data!); | ||
| } | ||
| exit(0); | ||
| } else { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also update flutter version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Dart constraint will take care of that, but we could.