Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unpin DDS and roll pub packages #147925

Merged
merged 3 commits into from May 8, 2024
Merged

Unpin DDS and roll pub packages #147925

merged 3 commits into from May 8, 2024

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented May 7, 2024

DDS was temporarily pinned to 4.1.0 because 4.2.0 triggered some test failures (see #147250). Those failures should be fixed by vm_service 14.2.2, so this unpins DDS and rolls both of these packages (along with devtools_shared, which is a DDS dependency).

(If the bot updates vm_service before this is done, I can rebase over that will reduce the size of this PR to just a few files)

DDS was temporarily pinned to 4.1.0 because 4.2.0 triggered some test failures. Those failures should be fixed by vm_service 14.2.2, so this unpins DDS and rolls both of these packages (along with devtools_shared, which is a DDS dependency).
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. a: internationalization Supporting other languages or locales. (aka i18n) d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos platform-web Web applications specifically f: integration_test The flutter/packages/integration_test plugin labels May 7, 2024
@DanTup
Copy link
Contributor Author

DanTup commented May 7, 2024

@bkonyi looks like the vm_service changes are failing some tests here:

 01:28 +4008 ~5: test/general.shard\vmservice_test.dart: setAssetDirectory forwards arguments correctly
 01:28 +4009 ~5: test/general.shard\vmservice_test.dart: setAssetDirectory forwards arguments correctly
 01:28 +4009 ~5 -1: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4009 ~5 -1: test/general.shard\vmservice_test.dart: setAssetDirectory forwards arguments correctly [E]
   _flutter.setAssetBundlePath: (-32000) Service connection disposed
   
 01:28 +4010 ~5 -1: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4010 ~5 -2: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4010 ~5 -2: test/general.shard\vmservice_test.dart: flushUIThreadTasks forwards arguments correctly [E]
   _flutter.flushUIThreadTasks: (-32000) Service connection disposed
   
 01:28 +4011 ~5 -2: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4012 ~5 -2: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4013 ~5 -2: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4014 ~5 -2: test/general.shard\version_test.dart: version does not call git if a .version.json file exists
 01:28 +4015 ~5 -2: test/general.shard\vmservice_test.dart: getFlutterViews polls until a view is returned

Example failing test is this one:

testWithoutContext('setAssetDirectory forwards arguments correctly', () async {

I can repro locally, but it's not clear to me if what the test is doing is reasonable or not. It provides an empty stream as the input stream, which causes the method being tested to immediately be terminated as the service is disposed. I feel like it shouldn't be providing a stream that is closed before it has done its testing.. possibly it could close the stream after setAssetDirectory completes instead.

Thoughts? I can try the above, but want to make sure you don't think vm_service should handle this kind of code.

@bkonyi
Copy link
Contributor

bkonyi commented May 7, 2024

It provides an empty stream as the input stream, which causes the method being tested to immediately be terminated as the service is disposed. I feel like it shouldn't be providing a stream that is closed before it has done its testing.. possibly it could close the stream after setAssetDirectory completes instead.

Yeah, this is a bad test. We should be using a FakeVmService that extends VmService and override the methods that are actually being called to return dummy data (I had to do something similar in google3).

@DanTup
Copy link
Contributor Author

DanTup commented May 7, 2024

Got it, I'll have a go :)

@DanTup
Copy link
Contributor Author

DanTup commented May 7, 2024

@christopherfujino @bkonyi I think this is good now (there's one bot outstanding as I type this, but the rest are green). There are some manual changes besides the pubspec changes to tidy up and fix some tests. You can type ".dart" in the file filter when looking at the changes to easily see the non-pubspec changes.

(cc @elliette in case you're interested in when this lands, it includes your permission-to-resume changes)

@DanTup DanTup mentioned this pull request May 7, 2024
Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DanTup DanTup merged commit cea1d6f into flutter:master May 8, 2024
136 checks passed
@DanTup DanTup deleted the unpin-dds branch May 8, 2024 09:35
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants