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

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 21 additions & 21 deletions packages/flutter_goldens/lib/flutter_goldens.dart
Expand Up @@ -103,10 +103,7 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
this.skiaClient, {
this.fs = const LocalFileSystem(),
this.platform = const LocalPlatform(),
}) : assert(basedir != null),
assert(skiaClient != null),
assert(fs != null),
assert(platform != null);
});

/// The directory to which golden file URIs will be resolved in [compare] and
/// [update], cannot be null.
Expand Down Expand Up @@ -231,8 +228,8 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
/// purposes only.
static Future<FlutterPostSubmitFileComparator> fromDefaultComparator(
final Platform platform, {
SkiaGoldClient goldens,
LocalFileComparator defaultComparator,
SkiaGoldClient? goldens,
LocalFileComparator? defaultComparator,
}) async {

defaultComparator ??= goldenFileComparator as LocalFileComparator;
Expand Down Expand Up @@ -326,9 +323,9 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
/// purposes only.
static Future<FlutterGoldenFileComparator> fromDefaultComparator(
final Platform platform, {
SkiaGoldClient goldens,
LocalFileComparator defaultComparator,
final Directory testBasedir,
SkiaGoldClient? goldens,
LocalFileComparator? defaultComparator,
Directory? testBasedir,
}) async {

defaultComparator ??= goldenFileComparator as LocalFileComparator;
Expand All @@ -353,7 +350,7 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
// Some contributors may not have permission on Cirrus to decrypt the
// service account.
onCirrusWithPermission =
!platform.environment['GOLD_SERVICE_ACCOUNT'].startsWith('ENCRYPTED');
!platform.environment['GOLD_SERVICE_ACCOUNT']!.startsWith('ENCRYPTED');
}
final bool onLuci = platform.environment.containsKey('SWARMING_TASK_ID');
if (onCirrusWithPermission || onLuci) {
Expand Down Expand Up @@ -457,7 +454,7 @@ class _UnauthorizedFlutterPreSubmitComparator extends FlutterPreSubmitFileCompar
// low.
skiaClient.getExpectations();
final String testName = skiaClient.cleanTestName(golden.path);
final List<String> testExpectations = skiaClient.expectations[testName];
final List<String>? testExpectations = skiaClient.expectations[testName];
if (testExpectations == null) {
// This is a new test.
print('No expectations provided by Skia Gold for test: $golden. '
Expand Down Expand Up @@ -502,8 +499,7 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
final Uri basedir,
final SkiaGoldClient skiaClient,
this.reason,
) : assert(reason != null),
super(basedir, skiaClient);
) : super(basedir, skiaClient);

/// Describes the reason for using the [FlutterSkippingFileComparator].
///
Expand All @@ -514,12 +510,12 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
/// relative path resolution of the default [goldenFileComparator].
static FlutterSkippingFileComparator fromDefaultComparator(
String reason, {
LocalFileComparator defaultComparator,
LocalFileComparator? defaultComparator,
}) {
defaultComparator ??= goldenFileComparator as LocalFileComparator;
const FileSystem fs = LocalFileSystem();
final Uri basedir = defaultComparator.basedir;
final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir));
final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir), ci: ContinuousIntegrationEnvironment.luci,);
return FlutterSkippingFileComparator(basedir, skiaClient, reason);
}

Expand All @@ -532,7 +528,7 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
}

@override
Future<void> update(Uri golden, Uint8List imageBytes) => null;
Future<void> update(Uri golden, Uint8List imageBytes) async {}

/// Decides, based on the current environment, if this comparator should be
/// used.
Expand Down Expand Up @@ -596,9 +592,9 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
/// visible for testing purposes only.
static Future<FlutterGoldenFileComparator> fromDefaultComparator(
final Platform platform, {
SkiaGoldClient goldens,
LocalFileComparator defaultComparator,
Directory baseDirectory,
SkiaGoldClient? goldens,
LocalFileComparator? defaultComparator,
Directory? baseDirectory,
}) async {
defaultComparator ??= goldenFileComparator as LocalFileComparator;
baseDirectory ??= FlutterGoldenFileComparator.getBaseDirectory(
Expand All @@ -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!

ci: platform.environment.containsKey('CIRRUS_CI')
? ContinuousIntegrationEnvironment.cirrus
: ContinuousIntegrationEnvironment.luci,
);

try {
await goldens.getExpectations();
Expand All @@ -638,7 +638,7 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
Future<bool> compare(Uint8List imageBytes, Uri golden) async {
golden = _addPrefix(golden);
final String testName = skiaClient.cleanTestName(golden.path);
final List<String> testExpectations = skiaClient.expectations[testName];
final List<String>? testExpectations = skiaClient.expectations[testName];
if (testExpectations == null) {
// There is no baseline for this test
print('No expectations provided by Skia Gold for test: $golden. '
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_goldens/pubspec.yaml
Expand Up @@ -2,7 +2,7 @@ name: flutter_goldens

environment:
# The pub client defaults to an <2.0.0 sdk constraint which we need to explicitly overwrite.
sdk: ">=2.0.0-dev.68.0 <3.0.0"
sdk: ">=2.10.0-4.0.dev <2.10.0"

dependencies:
# To update these, use "flutter update-packages --force-upgrade".
Expand Down
3 changes: 3 additions & 0 deletions packages/flutter_goldens/test/flutter_goldens_test.dart
Expand Up @@ -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

import 'dart:async';
import 'dart:convert';
import 'dart:core';
Expand Down Expand Up @@ -75,6 +76,7 @@ void main() {
process: process,
platform: platform,
httpClient: mockHttpClient,
ci: ContinuousIntegrationEnvironment.luci,
);
});

Expand Down Expand Up @@ -148,6 +150,7 @@ void main() {
process: process,
platform: platform,
httpClient: mockHttpClient,
ci: ContinuousIntegrationEnvironment.luci
);

when(process.run(
Expand Down
71 changes: 29 additions & 42 deletions packages/flutter_goldens_client/lib/skia_client.dart
Expand Up @@ -35,13 +35,9 @@ class SkiaGoldClient {
this.fs = const LocalFileSystem(),
this.process = const LocalProcessManager(),
this.platform = const LocalPlatform(),
this.ci,
io.HttpClient httpClient,
}) : assert(workDirectory != null),
assert(fs != null),
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.

io.HttpClient? httpClient,
}) : httpClient = httpClient ?? io.HttpClient();

/// The file system to use for storing the local clone of the repository.
///
Expand Down Expand Up @@ -83,7 +79,7 @@ class SkiaGoldClient {
/// [_UnauthorizedFlutterPreSubmitComparator] to test against golden masters
/// maintained in the Flutter Gold dashboard.
Map<String, List<String>> get expectations => _expectations;
Map<String, List<String>> _expectations;
late Map<String, List<String>> _expectations;

/// The local [Directory] where the Flutter repository is hosted.
///
Expand All @@ -93,13 +89,13 @@ class SkiaGoldClient {
/// The path to the local [Directory] where the goldctl tool is hosted.
///
/// Uses the [platform] environment in this implementation.
String/*!*/ get _goldctl => platform.environment[_kGoldctlKey];
String get _goldctl => platform.environment[_kGoldctlKey]!;

/// The path to the local [Directory] where the service account key is
/// hosted.
///
/// Uses the [platform] environment in this implementation.
String/*!*/ get _serviceAccount => platform.environment[_kServiceAccountKey];
String get _serviceAccount => platform.environment[_kServiceAccountKey]!;

/// Prepares the local work space for golden file testing and calls the
/// goldctl `auth` command.
Expand All @@ -116,9 +112,9 @@ class SkiaGoldClient {
return;

List<String> authArguments;
/*late*/ String failureContext;
String failureContext;

switch (ci/*!*/) {
switch (ci) {
case ContinuousIntegrationEnvironment.luci:
authArguments = <String>[
'auth',
Expand Down Expand Up @@ -267,9 +263,6 @@ class SkiaGoldClient {
/// The [testName] and [goldenFile] parameters reference the current
/// comparison being evaluated by the [FlutterPostSubmitFileComparator].
Future<bool> imgtestAdd(String testName, File goldenFile) async {
assert(testName != null);
assert(goldenFile != null);

final List<String> imgtestArguments = <String>[
'imgtest', 'add',
'--work-dir', workDirectory
Expand Down Expand Up @@ -361,9 +354,6 @@ class SkiaGoldClient {
/// The [testName] and [goldenFile] parameters reference the current
/// comparison being evaluated by the [_AuthorizedFlutterPreSubmitComparator].
Future<void> tryjobAdd(String testName, File goldenFile) async {
assert(testName != null);
assert(goldenFile != null);

final List<String> imgtestArguments = <String>[
'imgtest', 'add',
'--work-dir', workDirectory
Expand Down Expand Up @@ -410,9 +400,6 @@ class SkiaGoldClient {
/// comparison being evaluated by the
/// [_UnauthorizedFlutterPreSubmitComparator].
Future<bool> imgtestCheck(String testName, File goldenFile) async {
assert(testName != null);
assert(goldenFile != null);

final List<String> imgtestArguments = <String>[
'imgtest', 'check',
'--work-dir', workDirectory
Expand Down Expand Up @@ -440,15 +427,15 @@ class SkiaGoldClient {
);
const String mainKey = 'master';
const String temporaryKey = 'master_str';
String rawResponse;
late String rawResponse;
try {
final io.HttpClientRequest request = await httpClient.getUrl(requestForExpectations);
final io.HttpClientResponse response = await request.close();
rawResponse = await utf8.decodeStream(response);
final dynamic jsonResponse = json.decode(rawResponse);
if (jsonResponse is! Map<String, dynamic>)
throw const FormatException('Skia gold expectations do not match expected format.');
final Map<String, dynamic> skiaJson = (jsonResponse[mainKey] ?? jsonResponse[temporaryKey]) as Map<String, dynamic>;
final Map<String, dynamic>? skiaJson = (jsonResponse[mainKey] ?? jsonResponse[temporaryKey]) as Map<String, dynamic>?;
if (skiaJson == null)
throw FormatException('Skia gold expectations are missing the "$mainKey" key (and also doesn\'t have "$temporaryKey")! Available keys: ${jsonResponse.keys.join(", ")}');
skiaJson.forEach((String key, dynamic value) {
Expand Down Expand Up @@ -506,7 +493,7 @@ class SkiaGoldClient {
Future<bool> testIsIgnoredForPullRequest(String pullRequest, String testName) async {
bool ignoreIsActive = false;
testName = cleanTestName(testName);
String rawResponse;
late String rawResponse;
await io.HttpOverrides.runWithHttpOverrides<Future<void>>(() async {
final Uri requestForIgnores = Uri.parse(
'https://flutter-gold.skia.org/json/ignores'
Expand Down Expand Up @@ -565,7 +552,7 @@ class SkiaGoldClient {
Future<bool> isValidDigestForExpectation(String expectation, String testName) async {
bool isValid = false;
testName = cleanTestName(testName);
String rawResponse;
late String rawResponse;
await io.HttpOverrides.runWithHttpOverrides<Future<void>>(() async {
final Uri requestForDigest = Uri.parse(
'https://flutter-gold.skia.org/json/details?test=$testName&digest=$expectation'
Expand Down Expand Up @@ -653,20 +640,20 @@ class SkiaGoldClient {
/// Returns a list of arguments for initializing a tryjob based on the testing
/// environment.
List<String> getCIArguments() {
/*late*/ String/*!*/ pullRequest;
/*late*/ String/*!*/ jobId;
/*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


switch (ci/*!*/) {
switch (ci) {
case ContinuousIntegrationEnvironment.luci:
jobId = platform.environment['LOGDOG_STREAM_PREFIX'].split('/').last;
final List<String> refs = platform.environment['GOLD_TRYJOB'].split('/');
jobId = platform.environment['LOGDOG_STREAM_PREFIX']!.split('/').last;
final List<String> refs = platform.environment['GOLD_TRYJOB']!.split('/');
pullRequest = refs[refs.length - 2];
cis = 'buildbucket';
break;
case ContinuousIntegrationEnvironment.cirrus:
pullRequest = platform.environment['CIRRUS_PR'];
jobId = platform.environment['CIRRUS_TASK_ID'];
pullRequest = platform.environment['CIRRUS_PR']!;
jobId = platform.environment['CIRRUS_TASK_ID']!;
cis = 'cirrus';
break;
}
Expand All @@ -685,17 +672,17 @@ class SkiaGoldHttpOverrides extends io.HttpOverrides {}
/// A digest returned from a request to the Flutter Gold dashboard.
class SkiaGoldDigest {
const SkiaGoldDigest({
this.imageHash,
this.paramSet,
this.testName,
this.status,
required this.imageHash,
required this.paramSet,
required this.testName,
required this.status,
});

/// Create a digest from requested JSON.
factory SkiaGoldDigest.fromJson(Map<String, dynamic> json) {
return SkiaGoldDigest(
imageHash: json['digest'] as String,
paramSet: Map<String, dynamic>.from(json['paramset'] as Map<String, dynamic> ??
paramSet: Map<String, dynamic>.from(json['paramset'] as Map<String, dynamic>? ??
<String, List<String>>{
'Platform': <String>[],
'Browser' : <String>[],
Expand All @@ -706,16 +693,16 @@ class SkiaGoldDigest {
}

/// Unique identifier for the image associated with the digest.
final String/*!*/ imageHash;
final String imageHash;

/// Parameter set for the given test, e.g. Platform : Windows.
final Map<String, dynamic>/*!*/ paramSet;
final Map<String, dynamic> paramSet;

/// Test name associated with the digest, e.g. positive or un-triaged.
final String/*!*/ testName;
final String testName;

/// Status of the given digest, e.g. positive or un-triaged.
final String/*!*/ status;
final String status;

/// Validates a given digest against the current testing conditions.
bool isValid(Platform platform, String name, String expectation) {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_goldens_client/pubspec.yaml
Expand Up @@ -2,7 +2,7 @@ name: flutter_goldens_client

environment:
# The pub client defaults to an <2.0.0 sdk constraint which we need to explicitly overwrite.
sdk: ">=2.0.0-dev.68.0 <3.0.0"
sdk: ">=2.10.0-4.0.dev <2.10.0 <3.0.0"

dependencies:
# To update these, use "flutter update-packages --force-upgrade".
Expand Down