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

[flutter_tools] migrate flutter_goldens, flutter_goldens client to null-safety #64201

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Aug 19, 2020

Description

Unblock migration of flutter_test in #62886

@flutter-dashboard flutter-dashboard bot added team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Aug 19, 2020
@jonahwilliams jonahwilliams changed the title Update null safety flutter goldens [flutter_tools] migrate flutter_goldens, flutter_goldens client to null-safety Aug 20, 2020
@jonahwilliams jonahwilliams marked this pull request as ready for review August 20, 2020 00:01
assert(process != null),
assert(platform != null),
httpClient = httpClient ?? io.HttpClient();
required this.ci,
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to make this required since we don't handle null in the switch below.

/*late*/ String cis;
String pullRequest;
String jobId;
String cis;
Copy link
Member Author

Choose a reason for hiding this comment

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

yay definite assignment analysis

@@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// @dart = 2.8
Copy link
Member Author

Choose a reason for hiding this comment

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

This test uses too much mockito

@@ -611,7 +607,11 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
baseDirectory.createSync(recursive: true);
}

goldens ??= SkiaGoldClient(baseDirectory);
goldens ??= SkiaGoldClient(baseDirectory,
Copy link
Contributor

Choose a reason for hiding this comment

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

The local comparator is used in a non-ci environment, so neither of these would be correct. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

WDUT about ContinuousIntegrationEnvironment.none ?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. The switch statements will want it to be accounted for, it'll just be a pass through then. :) Thanks!

@jonahwilliams
Copy link
Member Author

need to make some of these APIs nullable

@jonahwilliams
Copy link
Member Author

I needed to update the goldens library in flutter_test that this package depends on

@jonahwilliams
Copy link
Member Author

this is ready for another review

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@fluttergithubbot fluttergithubbot merged commit 41e553b into flutter:master Aug 24, 2020
smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team Infra upgrades, team productivity, code health, technical debt. See also team: labels. 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

4 participants