nnbd for integration_test #3158

dnfield · 2020-10-15T00:53:16Z

TBD: What version/changelog changes to make.

blasten · 2020-10-15T00:57:04Z

0.10.0-nullsafety?

dnfield · 2020-10-15T00:58:02Z

Are we going to use teh -nullsafety for this? I was under the impression that that is a temporary designation for special packages and should not be widely used.

dnfield · 2020-10-15T00:58:15Z

/cc @jakemac53 @jonahwilliams @natebosch

blasten · 2020-10-15T00:59:40Z

Yes, the -nullsafety prerelease is temporary. The general guidance is to consider this as a breaking change (major version bump), so you can clean up/refine the API as needed.

CareF · 2020-10-15T01:14:21Z

packages/integration_test/example/pubspec.yaml

 environment:
-  sdk: ">=2.1.0 <3.0.0"
+  sdk: '>=2.10.0-56.0.dev <3.0.0'
  flutter: ">=1.6.7 <2.0.0"


Should we also update flutter version?

I think the Dart constraint will take care of that, but we could.

packages/integration_test/lib/_callback_web.dart

CareF · 2020-10-15T01:26:05Z

packages/integration_test/lib/integration_test.dart

+    @visibleForTesting vm.VmService? vmService,
  }) async {
-    assert(streams != null);
+    assert(streams != null); // ignore: unnecessary_null_comparison


This line seems no longer necessary when we have null-safety.

It's still useful when not in sound null safety e.g. importing a non nnbd package

nit: does it make sense to merge to asserts?

@nturgut I'm not sure I'm understanding what merge asserts would be in this context...

it's very nit :)

assert(streams !=null && streams. isNotEmpty, 'Non empty stream is required')

CareF · 2020-10-15T01:27:45Z

packages/integration_test/lib/integration_test.dart

-      );
-    }
-    await _vmService.setVMTimelineFlags(streams);
+    vmService ??= await _vmService;


I think this should be if (vmService != null) _cachedVmService = vmService, so that traceTimeline will use the provided vmService. And to make it logically more consistent I would change the next line back to await _vmService.setVMTimelineFlags(streams);

I've made the private member a future. I'm also not clear on this, I assumed the param was only for testing.

But this parameter is intended to replace the standard vmService for testing purposes. If I understand it correctly, your version traceTimeline will still call the _cachedVmService obtained by vm_io.vmServiceConnectUri, but what it should call is the vmService provided here.

Hmm. We should add it as a param there then or move it to the ctor/ensureInitialized It's pretty strange to have a method alter a private member for tests.

I've cleaned this up a bit, let me know what you think.

CareF · 2020-10-15T01:32:29Z

packages/integration_test/lib/integration_test_driver.dart

+  String? destinationDirectory,
 }) async {
-  assert(testOutputFilename != null);
+  assert(testOutputFilename != null); // ignore: unnecessary_null_comparison


This line seems no longer necessary when we have null-safety.

Same comment as above - these are still helpful when working in mixed/unsound mode.

dnfield · 2020-10-15T06:23:47Z

This is going to have problems because of package:mockito and package:vm_service.

CareF

LGTM when the mockVM issue is solved.

dnfield · 2020-10-15T15:57:36Z

So we don't really need mockito for this, and the story around using that for NNBD is still being developed so we can just avoid having it block this.

Still will be blocked on migration of package:vm_service

nturgut · 2020-10-15T23:44:04Z

packages/integration_test/lib/integration_test.dart


-  vm.VmService _vmService;
+  vm.VmService? _cachedVmService;
+  Future<vm.VmService> get _vmService async {


Is this change related to nnbd?

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.

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 😅 )

nturgut · 2020-10-15T23:48:39Z

packages/integration_test/pubspec.yaml


 environment:
-  sdk: ">=2.2.2 <3.0.0"
+  sdk: '>=2.10.0-56.0.dev <3.0.0'


I wonder if this will break the flutter/engine tests, when we want to update e2e version?

https://github.com/flutter/engine/blob/master/e2etests/web/regular_integration_tests/pubspec.yaml#L5 For integration tests

web_ui: https://github.com/flutter/engine/blob/master/lib/web_ui/pubspec.yaml#L5

There's no rush on this, and we won't be able to move forward with it realistically until package:vm_service is migrated, which probably won't happen for at least a couple more weeks.

This constraint should be ok, but by the time we'd publish this it should have a real 2.11 (not .dev) constraint.

thanks for the info! :)

nturgut

I added a few comments. I think the best way would be to test a web engine integration test locally with the new version (since the existing tests use the old version we won't know if they fail until we try to upgrade)

dnfield · 2020-11-12T23:15:43Z

I'm going to close this, since this package is moving to the flutter repo.

ferhatb · 2020-11-13T01:38:40Z

@dnfield this is blocking us from migrating our tests to non-nullable. Is there a separate issue that tracks move to flutter repo ?

dnfield · 2020-11-13T03:48:27Z

It's been moved already. The real blocker for this is that vm_service and then flutter_driver need to be migrated

nnbd for integration_test

a101150

dnfield requested review from CareF, blasten and nturgut October 15, 2020 00:53

google-cla bot added the cla: yes label Oct 15, 2020

CareF reviewed Oct 15, 2020

View reviewed changes

packages/integration_test/lib/_callback_web.dart Show resolved Hide resolved

CareF reviewed Oct 15, 2020

View reviewed changes

dnfield added 2 commits October 14, 2020 23:19

Version bump, clean up binding test/initialization

3aa6756

merge

ed86ddb

CareF approved these changes Oct 15, 2020

View reviewed changes

drop mockito

e47169b

mvanbeusekom mentioned this pull request Oct 15, 2020

[url_launcher] migrate url_launcher package to null safety #3148

Merged

13 tasks

nturgut reviewed Oct 15, 2020

View reviewed changes

nturgut approved these changes Oct 16, 2020

View reviewed changes

dnfield closed this Nov 12, 2020

pedromassangocode mentioned this pull request Nov 30, 2020

[integration_test] Needs null safety migration flutter/flutter#71379

Closed

dnfield mentioned this pull request Jan 28, 2021

NNBD integration_test flutter/flutter#74922

Merged

8 tasks

nnbd for integration_test #3158

nnbd for integration_test #3158

Uh oh!

Conversation

dnfield commented Oct 15, 2020

Uh oh!

blasten commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Oct 15, 2020

Uh oh!

dnfield commented Oct 15, 2020

Uh oh!

blasten commented Oct 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

CareF Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Oct 15, 2020

Uh oh!

CareF left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Oct 15, 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!

nturgut Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nturgut left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Nov 12, 2020

Uh oh!

ferhatb commented Nov 13, 2020

Uh oh!

dnfield commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

blasten commented Oct 15, 2020 •

edited

Loading

CareF Oct 15, 2020 •

edited

Loading

nturgut Oct 16, 2020 •

edited

Loading