Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
updated error logic to be more readable and added docArtifact test
  • Loading branch information
jesswrd committed Oct 29, 2024
commit 80f0902721fedaa0b2e7ec7a994812519b44ac8d
32 changes: 22 additions & 10 deletions script/tool/lib/src/gradle_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,6 @@ apply plugin: "com.google.cloud.artifactregistry.gradle-plugin"
/// GP stands for the gradle plugin method of flutter tooling inclusion.
@visibleForTesting
static String exampleSettingsArtifactHubStringGP = '''
// See $artifactHubDocumentationString for more info.
plugins {
id "dev.flutter.flutter-plugin-loader" version "1.0.0"
// ...other plugins
Expand All @@ -243,6 +242,8 @@ plugins {
r'apply.*plugin.*com\.google\.cloud\.artifactregistry\.gradle-plugin');
final RegExp artifactRegistryPluginApplyRegexGP = RegExp(
r'id.*com\.google\.cloud\.artifactregistry\.gradle-plugin.*version.*\b\d+\.\d+\.\d+\b');
final RegExp artifactRegistryPluginApplyDeclarativeRegex = RegExp(
r'\bpluginManagement\b');


final bool documentationPresent = gradleLines
Expand All @@ -251,19 +252,30 @@ plugins {
.any((String line) => artifactRegistryDefinitionRegex.hasMatch(line));
final bool artifactRegistryPluginApplied = gradleLines
.any((String line) => artifactRegistryPluginApplyRegex.hasMatch(line));
final bool artifactRegistryPluginAppliedGP = gradleLines
final bool declarativeArtifactRegistryApplied = gradleLines
.any((String line) => artifactRegistryPluginApplyRegexGP.hasMatch(line));
final bool declarativePluginBlockApplied = gradleLines
.any((String line) => artifactRegistryPluginApplyDeclarativeRegex.hasMatch(line));

final bool imperativeArtifactRegistryApplied = artifactRegistryDefined && artifactRegistryPluginApplied;

final bool validArtifactConfiguration = documentationPresent &&
((artifactRegistryDefined && artifactRegistryPluginApplied) || artifactRegistryPluginAppliedGP);
(imperativeArtifactRegistryApplied || declarativeArtifactRegistryApplied);

if (!validArtifactConfiguration && !artifactRegistryPluginAppliedGP) {
printError('Failed Artifact Hub validation. Include the following in '
'example root settings.gradle:\n$exampleSettingsArtifactHubStringGP');
}
if (!validArtifactConfiguration && !(artifactRegistryDefined && artifactRegistryPluginApplied)) {
printError('Failed Artifact Hub validation. Include the following in '
'example root settings.gradle:\n$exampleRootSettingsArtifactHubString');
if (!validArtifactConfiguration) {
printError('Failed Artifact Hub validation.');
if (!documentationPresent) {
printError('The link to the Artifact Hub documentation is missing. Include the following in '
'example root settings.gradle:\n// See $artifactHubDocumentationString for more info.');
}
if (artifactRegistryDefined || artifactRegistryPluginApplied || !declarativePluginBlockApplied) {
printError('Include the following in '
'example root settings.gradle:\n$exampleRootSettingsArtifactHubString');
}
else if (!declarativeArtifactRegistryApplied){
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file needs dart format run on it. (CI currently doesn't check formatting for script/tool, which is a bug.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

printError('Include the following in '
'example root settings.gradle:\n$exampleSettingsArtifactHubStringGP');
}
}
return validArtifactConfiguration;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the only thing is missing is the documentation, before it would print an example of what needed to be included that included the documentation, whereas now it would fail without any actionable message.

Copy link
Contributor Author

@jesswrd jesswrd Oct 28, 2024

Choose a reason for hiding this comment

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

I just tried out only removing only the documentation, and it does include an error message. I have a check for documentationPresent when validArtifactConfiguration is deinfed, and check for !validArtifactConfiguration when an error should be printed. Could you further explain why it would fail?

}
Expand Down
47 changes: 40 additions & 7 deletions script/tool/test/gradle_check_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ ${includeArtifactHub ? GradleCheckCommand.exampleRootSettingsArtifactHubString :
void writeFakeExampleSettingsGradle(
RepositoryPackage package, {
bool includeArtifactHub = true,
bool includeArtifactDocumentation = true,
}) {
final File settingsGradle = package
.platformDirectory(FlutterPlatform.android)
Expand All @@ -215,7 +216,6 @@ ${includeArtifactHub ? GradleCheckCommand.exampleRootSettingsArtifactHubString :
/// configuration without the artifact hub env variable.
/// GP stands for the gradle plugin method of flutter tooling inclusion.
const String exampleSettingsWithoutArtifactHubStringGP = '''
// See ${GradleCheckCommand.artifactHubDocumentationString} for more info.
plugins {
id "dev.flutter.flutter-plugin-loader" version "1.0.0"
// ...other plugins
Expand All @@ -240,6 +240,8 @@ pluginManagement {
gradlePluginPortal()
}
}

${includeArtifactDocumentation ? '// See ${GradleCheckCommand.artifactHubDocumentationString} for more info.' : ''}
${includeArtifactHub ? GradleCheckCommand.exampleSettingsArtifactHubStringGP : exampleSettingsWithoutArtifactHubStringGP}
include ":app"
''');
Expand Down Expand Up @@ -328,6 +330,7 @@ dependencies {
String? kotlinVersion,
bool includeBuildArtifactHub = true,
bool includeSettingsArtifactHub = true,
bool includeDocumentationArtifactHub = true,
}) {
writeFakeExampleTopLevelBuildGradle(
package,
Expand All @@ -341,6 +344,7 @@ dependencies {
writeFakeExampleSettingsGradle(
package,
includeArtifactHub: includeSettingsArtifactHub,
includeArtifactDocumentation: includeDocumentationArtifactHub,
);
}

Expand Down Expand Up @@ -858,13 +862,9 @@ dependencies {
output,
isNot(contains(GradleCheckCommand.exampleRootGradleArtifactHubString)),
);
expect(
output,
isNot(contains(GradleCheckCommand.exampleSettingsArtifactHubStringGP)),
);
});

test('contains declarative method of applying gradle plugins', () async {
test('prints error for declarative method of applying gradle plugins', () async {
const String packageName = 'a_package';
final RepositoryPackage package =
createFakePackage('a_package', packagesDir);
Expand All @@ -875,7 +875,9 @@ dependencies {
pluginName: packageName,
// ignore: avoid_redundant_argument_values
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we always want to have the tests be explicit about all of their arguments, then let's make the test helper arguments be required instead of having to annotate all of the calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see the version of the code where includeBuildArtifactHub and includeSettingsDocumentationArtifactHub are required without defaults and the // ignore: avoid_redundant_argument_values are removed. Did you need to push a change?

Copy link
Contributor Author

@jesswrd jesswrd Oct 30, 2024

Choose a reason for hiding this comment

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

I added required and removed the annotations for writeFakeExampleBuildGradleGP but not for writeFakeExampleBuildGradles. Keeping args optional for writeFakeExampleBuildGradles as it is used in unrelated tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that writeFakeExampleBuildGradles will need to be modified in the version of your pr that removes the "or" condition from the older way and the way you are replacing but for now can be left alone.

includeBuildArtifactHub: true,
includeSettingsArtifactHub: false);
includeSettingsArtifactHub: false,
// ignore: avoid_redundant_argument_values
includeDocumentationArtifactHub: true);
writeFakeManifest(example, isApp: true);

Error? commandError;
Expand All @@ -896,6 +898,37 @@ dependencies {
isNot(contains(GradleCheckCommand.exampleRootSettingsArtifactHubString)),
);
});

test('error message is printed when documentation link is missing', () async {
const String packageName = 'a_package';
final RepositoryPackage package =
createFakePackage('a_package', packagesDir);
writeFakePluginBuildGradle(package, includeLanguageVersion: true);
writeFakeManifest(package);
final RepositoryPackage example = package.getExamples().first;
writeFakeExampleBuildGradleGP(example,
pluginName: packageName,
// ignore: avoid_redundant_argument_values
includeBuildArtifactHub: true,
// ignore: avoid_redundant_argument_values
includeSettingsArtifactHub: true,
includeDocumentationArtifactHub: false);
writeFakeManifest(example, isApp: true);

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['gradle-check'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains(GradleCheckCommand.artifactHubDocumentationString),
]),
);
});
});

group('Kotlin version check', () {
Expand Down