Skip to content

Migrate Flutter gallery test to null safety #69048

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.

Sign up for GitHub

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

Jump to bottom
Merged
blasten merged 20 commits into from
Nov 7, 2020
Merged

Migrate Flutter gallery test to null safety #69048

blasten merged 20 commits into from
Nov 7, 2020

Conversation

@blasten
Copy link

@blasten blasten commented Oct 26, 2020

Fixes #66395

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 26, 2020
@google-cla google-cla bot added the cla: yes label Oct 26, 2020
@blasten
Copy link
Author

blasten commented Oct 26, 2020

@jonahwilliams I'm looking at merging this PR, which requires passing the --enable-experiment=non-nullable flag to the tool. Any thoughts?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 26, 2020

You'll need to either wait for the flag flip when --enable-experiment is no longer necessary, or go through all of the integration tests and provide the flag manually. My understanding is that the flag flip is happening fairly soon, and this change does not need to be landed before that point.

@leafpetersen or @jakemac53 to confirm?

@jakemac53
Copy link
Contributor

jakemac53 commented Oct 26, 2020

We are actively working on unblocking the flag flip so yes I would probably just wait if that is possible.

@blasten
Copy link
Author

blasten commented Oct 26, 2020

or go through all of the integration tests and provide the flag manually

Do you mean the Flutter gallery ones only? @jonahwilliams

@blasten
Copy link
Author

blasten commented Oct 26, 2020

Alright. Sounds good.

@pcsosinski
Copy link

pcsosinski commented Oct 26, 2020

gallery performance is a gate for being beta ready, so ideally we have this data asap

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 26, 2020

There are probably a hundred or so places that need --enable-experiment=non-nullable. Unless the beta plan is to ship the very first commit after the flag flip, waiting seems like a safe option to me.

@pcsosinski
Copy link

pcsosinski commented Oct 27, 2020

talked to @leafpetersen and @a-siva this morning, waiting for the flag flip PR to land is the plan.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 28, 2020

The test scripts themselves should not be migrated to null safety, otherwise you will get sound mode warnings about the test script dependencies. This is unimportant as we care about the app under test running in sound mode.

@pcsosinski
Copy link

pcsosinski commented Nov 2, 2020

the flag flip has landed, what's left to get this in?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Nov 2, 2020

Looks like this needs to be rebased

@pcsosinski
Copy link

pcsosinski commented Nov 2, 2020

@blasten are you able to rebase this PR?

@blasten
Copy link
Author

blasten commented Nov 2, 2020

@pcsosinski Done

@blasten blasten requested a review from jonahwilliams November 2, 2020 23:21
jonahwilliams
jonahwilliams reviewed Nov 2, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

You need to update the SDK constraint here:

https://github.com/flutter/flutter/blob/master/dev/integration_tests/flutter_gallery/pubspec.yaml#L5

I think it needs to be at least 2.12 to opt into null safety

@blasten
Copy link
Author

blasten commented Nov 2, 2020

@jonahwilliams I followed https://dart.dev/null-safety#configure-the-sdk-version

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Nov 4, 2020

We'd need new versions of all the plugins published with updated constraints

@tvolkert
Copy link
Contributor

tvolkert commented Nov 4, 2020

Seems to be blocked on #69771 and #69770 - correct?

@blasten
Copy link
Author

blasten commented Nov 4, 2020

I see. In the meanwhile, I will update the Dart SDK constraint for the plugins.

@blasten blasten requested a review from jonahwilliams November 6, 2020 01:05
jonahwilliams
jonahwilliams approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Overal changes LGTM

There is one more failure in the customer testing shard due to a constraint on intl ^0.16 which does not grab the 0.17 version since the overall version is < 1.0.

dev/integration_tests/flutter_gallery/analysis_options.yaml Outdated
analyzer:
exclude:
- build/**
enable-experiment:
Copy link
Contributor

@jonahwilliams jonahwilliams Nov 6, 2020

Choose a reason for hiding this comment

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

With the dart SDK constraint set to >=2.12.0-0 < 3 this shouldn't be necessary.

dev/integration_tests/flutter_gallery/lib/demo/material/buttons_demo.dart Outdated
icon: const Icon(Icons.add, size: 18.0),
label: const Text('DISABLED', semanticsLabel: 'DISABLED BUTTON 6'),
onPressed: null,
onPressed: () {},
Copy link
Contributor

@jonahwilliams jonahwilliams Nov 6, 2020

Choose a reason for hiding this comment

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

I think this should be null so that the button stays disabled.

Copy link
Author

@blasten blasten Nov 6, 2020

Choose a reason for hiding this comment

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

Done

@blasten blasten mentioned this pull request Nov 6, 2020
Closed
@jonahwilliams
Copy link
Contributor

jonahwilliams commented Nov 6, 2020

Fix for the font loader failure is flutter/engine#22370

@blasten
Copy link
Author

blasten commented Nov 6, 2020

@jonahwilliams I sent flutter/engine#22373. Should it be void or String??

@flutter flutter deleted a comment from fluttergithubbot Nov 6, 2020
@blasten blasten mentioned this pull request Nov 7, 2020
Closed
@blasten blasten merged commit 7ba775a into flutter:master Nov 7, 2020
@blasten blasten deleted the gallery_test_nnbd branch November 7, 2020 04:38
@blasten
Copy link
Author

blasten commented Nov 7, 2020

The BUILD file for the Gallery package needs to turn null safety on, then it should just work. fyi @renyou

mit-mit added a commit that referenced this pull request Nov 7, 2020
@mit-mit
Revert "Migrate Flutter gallery test to null safety (#69048)"
97b9229 
This reverts commit 7ba775a.
@mit-mit mit-mit mentioned this pull request Nov 7, 2020
Merged
mit-mit added a commit that referenced this pull request Nov 7, 2020
@mit-mit
Revert "Migrate Flutter gallery test to null safety (#69048)" (#70023)
e4d94f7 
This reverts commit 7ba775a.
@mit-mit
Copy link
Member

mit-mit commented Nov 9, 2020

This was reverted in #70023

@blasten blasten mentioned this pull request Nov 9, 2020
Merged
@xster xster mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Awaiting requested review from Piinks

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate Flutter Gallery test to NNBD

7 participants

@blasten @jonahwilliams @jakemac53 @pcsosinski @tvolkert @mit-mit @fluttergithubbot