From 7d2022eb2b92d1483ec7356e75f7fb06cc35bf99 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 20 Mar 2023 16:33:38 -0400 Subject: [PATCH 1/4] [tool] Check Java integration test configuration Rather than ignoring files by filename convention (*ActivityTest.java), look for the @DartIntegrationTest annotation that the test filter actually uses, to prevent accidentally excluding files. Also validate that the annotation is present when it should be, so that forgetting to add it doesn't cause unexplained hangs when running locally. --- script/tool/lib/src/native_test_command.dart | 132 +++++++++++++----- .../tool/test/native_test_command_test.dart | 71 +++++++++- 2 files changed, 165 insertions(+), 38 deletions(-) diff --git a/script/tool/lib/src/native_test_command.dart b/script/tool/lib/src/native_test_command.dart index af5f4df98e86..74fb729d5aa1 100644 --- a/script/tool/lib/src/native_test_command.dart +++ b/script/tool/lib/src/native_test_command.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'package:file/file.dart'; +import 'package:meta/meta.dart'; import 'package:platform/platform.dart'; import 'common/cmake.dart'; @@ -21,6 +22,14 @@ const String _iOSDestinationFlag = 'ios-destination'; const int _exitNoIOSSimulators = 3; +@visibleForTesting +const String misconfiguredJavaIntegrationTestErrorExplanation = + 'The following files use @RunWith(FlutterTestRunner.class) ' + 'but not @DartIntegrationTest, which will cause hangs when run with ' + 'this command. See ' + 'https://github.com/flutter/flutter/wiki/Plugin-Tests#enabling-android-ui-tests ' + 'for instructions.'; + /// The command to run native tests for plugins: /// - iOS and macOS: XCTests (XCUnitTest and XCUITest) /// - Android: JUnit tests @@ -211,7 +220,17 @@ this command. .existsSync(); } - bool exampleHasNativeIntegrationTests(RepositoryPackage example) { + _JavaTestInfo getJavaTestInfo(File testFile) { + final List contents = testFile.readAsLinesSync(); + return _JavaTestInfo( + usesFlutterTestRunner: contents.any((String line) => + line.trimLeft().startsWith('@RunWith(FlutterTestRunner.class)')), + hasDartIntegrationTestAnnotation: contents.any((String line) => + line.trimLeft().startsWith('@DartIntegrationTest'))); + } + + Map findIntegrationTestFiles( + RepositoryPackage example) { final Directory integrationTestDirectory = example .platformDirectory(FlutterPlatform.android) .childDirectory('app') @@ -220,25 +239,30 @@ this command. // There are two types of integration tests that can be in the androidTest // directory: // - FlutterTestRunner.class tests, which bridge to Dart integration tests - // - Purely native tests + // - Purely native integration tests // Only the latter is supported by this command; the former will hang if // run here because they will wait for a Dart call that will never come. // - // This repository uses a convention of putting the former in a - // *ActivityTest.java file, so ignore that file when checking for tests. - // Also ignore DartIntegrationTest.java, which defines the annotation used - // below for filtering the former out when running tests. + // Find all test files, and determine which kind of test they are based on + // the annotations they use. // - // If those are the only files, then there are no tests to run here. - return integrationTestDirectory.existsSync() && - integrationTestDirectory - .listSync(recursive: true) - .whereType() - .any((File file) { - final String basename = file.basename; - return !basename.endsWith('ActivityTest.java') && - basename != 'DartIntegrationTest.java'; - }); + // Ignore DartIntegrationTest.java, which defines the annotation used + // below for filtering the former out when running tests. + if (!integrationTestDirectory.existsSync()) { + return {}; + } + final Iterable integrationTestFiles = integrationTestDirectory + .listSync(recursive: true) + .whereType() + .where((File file) { + final String basename = file.basename; + return basename != 'DartIntegrationTest.java' && + basename != 'DartIntegrationTest.kt'; + }); + return { + for (final File file in integrationTestFiles) + file: getJavaTestInfo(file) + }; } final Iterable examples = plugin.getExamples(); @@ -247,10 +271,17 @@ this command. bool ranAnyTests = false; bool failed = false; bool hasMissingBuild = false; + bool hasMisconfiguredIntegrationTest = false; + // Iterate through all examples (in the rare case that there is more than + // on example); running any tests found for each one. Requirements on what + // tests are present are enforced at the overall package level, not a per- + // example level. E.g., it's fine for only one example to have unit tests. for (final RepositoryPackage example in examples) { final bool hasUnitTests = exampleHasUnitTests(example); - final bool hasIntegrationTests = - exampleHasNativeIntegrationTests(example); + final Map candidateIntegrationTestFiles = + findIntegrationTestFiles(example); + final bool hasIntegrationTests = candidateIntegrationTestFiles.values + .any((_JavaTestInfo info) => !info.hasDartIntegrationTestAnnotation); if (mode.unit && !hasUnitTests) { _printNoExampleTestsMessage(example, 'Android unit'); @@ -295,24 +326,41 @@ this command. if (runIntegrationTests) { // FlutterTestRunner-based tests will hang forever if run in a normal - // app build, since they wait for a Dart call from integration_test that - // will never come. Those tests have an extra annotation to allow + // app build, since they wait for a Dart call from integration_test + // that will never come. Those tests have an extra annotation to allow // filtering them out. - const String filter = - 'notAnnotation=io.flutter.plugins.DartIntegrationTest'; - - print('Running integration tests...'); - final int exitCode = await project.runCommand( - 'app:connectedAndroidTest', - arguments: [ - '-Pandroid.testInstrumentationRunnerArguments.$filter', - ], - ); - if (exitCode != 0) { - printError('$exampleName integration tests failed.'); - failed = true; + final List misconfiguredTestFiles = candidateIntegrationTestFiles + .entries + .where((MapEntry entry) => + entry.value.usesFlutterTestRunner && + !entry.value.hasDartIntegrationTestAnnotation) + .map((MapEntry entry) => entry.key) + .toList(); + if (misconfiguredTestFiles.isEmpty) { + // Ideally we would filter out @RunWith(FlutterTestRunner.class) + // tests directly, but there doesn't seem to be a way to filter based + // on annotation contents, so the DartIntegrationTest annotation was + // created as a proxy for that. + const String filter = + 'notAnnotation=io.flutter.plugins.DartIntegrationTest'; + + print('Running integration tests...'); + final int exitCode = await project.runCommand( + 'app:connectedAndroidTest', + arguments: [ + '-Pandroid.testInstrumentationRunnerArguments.$filter', + ], + ); + if (exitCode != 0) { + printError('$exampleName integration tests failed.'); + failed = true; + } + ranAnyTests = true; + } else { + hasMisconfiguredIntegrationTest = true; + printError('$misconfiguredJavaIntegrationTestErrorExplanation\n' + '${misconfiguredTestFiles.map((File f) => ' ${f.path}').join('\n')}'); } - ranAnyTests = true; } } @@ -322,6 +370,10 @@ this command. ? 'Examples must be built before testing.' : null); } + if (hasMisconfiguredIntegrationTest) { + return _PlatformResult(RunState.failed, + error: 'Misconfigured integration test.'); + } if (!mode.integrationOnly && !ranUnitTests) { printError('No unit tests ran. Plugins are required to have unit tests.'); return _PlatformResult(RunState.failed, @@ -622,3 +674,15 @@ class _PlatformResult { /// Ignored unless [state] is `failed`. final String? error; } + +/// The state of a .java test file. +class _JavaTestInfo { + const _JavaTestInfo( + {required this.usesFlutterTestRunner, + required this.hasDartIntegrationTestAnnotation}); + + /// Whether the test class uses the FlutterTestRunner runner. + final bool usesFlutterTestRunner; + //// Whether the test class has the @DartIntegrationTest annotation. + final bool hasDartIntegrationTestAnnotation; +} diff --git a/script/tool/test/native_test_command_test.dart b/script/tool/test/native_test_command_test.dart index f24d014bbfea..d08e8131c77c 100644 --- a/script/tool/test/native_test_command_test.dart +++ b/script/tool/test/native_test_command_test.dart @@ -8,6 +8,7 @@ import 'dart:io' as io; import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:file/memory.dart'; +import 'package:path/path.dart' as p; import 'package:flutter_plugin_tools/src/common/cmake.dart'; import 'package:flutter_plugin_tools/src/common/core.dart'; import 'package:flutter_plugin_tools/src/common/file_utils.dart'; @@ -511,9 +512,11 @@ void main() { }); test( - 'ignores Java integration test files associated with integration_test', + 'ignores Java integration test files using (or defining) DartIntegrationTest', () async { - createFakePlugin( + const String dartTestDriverRelativePath = + 'android/app/src/androidTest/java/io/flutter/plugins/plugin/FlutterActivityTest.java'; + final RepositoryPackage plugin = createFakePlugin( 'plugin', packagesDir, platformSupport: { @@ -522,11 +525,24 @@ void main() { extraFiles: [ 'example/android/gradlew', 'example/android/app/src/androidTest/java/io/flutter/plugins/DartIntegrationTest.java', - 'example/android/app/src/androidTest/java/io/flutter/plugins/plugin/FlutterActivityTest.java', - 'example/android/app/src/androidTest/java/io/flutter/plugins/plugin/MainActivityTest.java', + 'example/android/app/src/androidTest/java/io/flutter/plugins/DartIntegrationTest.kt', + 'example/$dartTestDriverRelativePath', ], ); + final File dartTestDriverFile = childFileWithSubcomponents( + plugin.getExamples().first.directory, + p.posix.split(dartTestDriverRelativePath)); + dartTestDriverFile.writeAsStringSync(''' +import io.flutter.plugins.DartIntegrationTest; +import org.junit.runner.RunWith; + +@DartIntegrationTest +@RunWith(FlutterTestRunner.class) +public class FlutterActivityTest { +} +'''); + await runCapturingPrint( runner, ['native-test', '--android', '--no-unit']); @@ -538,6 +554,53 @@ void main() { ); }); + test( + 'fails for Java integration tests Using FlutterTestRunner without @DartIntegrationTest', + () async { + const String dartTestDriverRelativePath = + 'android/app/src/androidTest/java/io/flutter/plugins/plugin/FlutterActivityTest.java'; + final RepositoryPackage plugin = createFakePlugin( + 'plugin', + packagesDir, + platformSupport: { + platformAndroid: const PlatformDetails(PlatformSupport.inline) + }, + extraFiles: [ + 'example/android/gradlew', + 'example/$dartTestDriverRelativePath', + ], + ); + + final File dartTestDriverFile = childFileWithSubcomponents( + plugin.getExamples().first.directory, + p.posix.split(dartTestDriverRelativePath)); + dartTestDriverFile.writeAsStringSync(''' +import io.flutter.plugins.DartIntegrationTest; +import org.junit.runner.RunWith; + +@RunWith(FlutterTestRunner.class) +public class FlutterActivityTest { +} +'''); + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['native-test', '--android', '--no-unit'], + errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + contains( + contains(misconfiguredJavaIntegrationTestErrorExplanation))); + expect( + output, + contains(contains( + 'example/android/app/src/androidTest/java/io/flutter/plugins/plugin/FlutterActivityTest.java'))); + }); + test('runs all tests when present', () async { final RepositoryPackage plugin = createFakePlugin( 'plugin', From b13618c763be698a27aa1f79714afaa629985a79 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 21 Mar 2023 10:57:37 -0400 Subject: [PATCH 2/4] Review comments --- script/tool/lib/src/native_test_command.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/script/tool/lib/src/native_test_command.dart b/script/tool/lib/src/native_test_command.dart index 74fb729d5aa1..dbf34a28d7c7 100644 --- a/script/tool/lib/src/native_test_command.dart +++ b/script/tool/lib/src/native_test_command.dart @@ -273,7 +273,7 @@ this command. bool hasMissingBuild = false; bool hasMisconfiguredIntegrationTest = false; // Iterate through all examples (in the rare case that there is more than - // on example); running any tests found for each one. Requirements on what + // one example); running any tests found for each one. Requirements on what // tests are present are enforced at the overall package level, not a per- // example level. E.g., it's fine for only one example to have unit tests. for (final RepositoryPackage example in examples) { @@ -681,7 +681,7 @@ class _JavaTestInfo { {required this.usesFlutterTestRunner, required this.hasDartIntegrationTestAnnotation}); - /// Whether the test class uses the FlutterTestRunner runner. + /// Whether the test class uses the FlutterTestRunner. final bool usesFlutterTestRunner; //// Whether the test class has the @DartIntegrationTest annotation. final bool hasDartIntegrationTestAnnotation; From 8b71cc87073371992289fe8c3bb319bc61a5bad2 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 21 Mar 2023 10:58:11 -0400 Subject: [PATCH 3/4] Fix comment --- script/tool/lib/src/native_test_command.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/script/tool/lib/src/native_test_command.dart b/script/tool/lib/src/native_test_command.dart index dbf34a28d7c7..760dac358c6e 100644 --- a/script/tool/lib/src/native_test_command.dart +++ b/script/tool/lib/src/native_test_command.dart @@ -683,6 +683,7 @@ class _JavaTestInfo { /// Whether the test class uses the FlutterTestRunner. final bool usesFlutterTestRunner; - //// Whether the test class has the @DartIntegrationTest annotation. + + /// Whether the test class has the @DartIntegrationTest annotation. final bool hasDartIntegrationTestAnnotation; } From 11595c8974e0babff30cdcc111b25ee6d23f0863 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 21 Mar 2023 11:00:29 -0400 Subject: [PATCH 4/4] Analysis fixes --- script/tool/lib/src/native_test_command.dart | 2 ++ script/tool/test/native_test_command_test.dart | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/script/tool/lib/src/native_test_command.dart b/script/tool/lib/src/native_test_command.dart index 760dac358c6e..f9967ca0523e 100644 --- a/script/tool/lib/src/native_test_command.dart +++ b/script/tool/lib/src/native_test_command.dart @@ -22,6 +22,8 @@ const String _iOSDestinationFlag = 'ios-destination'; const int _exitNoIOSSimulators = 3; +/// The error message logged when a FlutterTestRunner test is not annotated with +/// @DartIntegrationTest. @visibleForTesting const String misconfiguredJavaIntegrationTestErrorExplanation = 'The following files use @RunWith(FlutterTestRunner.class) ' diff --git a/script/tool/test/native_test_command_test.dart b/script/tool/test/native_test_command_test.dart index d08e8131c77c..ddbf6a50696b 100644 --- a/script/tool/test/native_test_command_test.dart +++ b/script/tool/test/native_test_command_test.dart @@ -8,12 +8,12 @@ import 'dart:io' as io; import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:file/memory.dart'; -import 'package:path/path.dart' as p; import 'package:flutter_plugin_tools/src/common/cmake.dart'; import 'package:flutter_plugin_tools/src/common/core.dart'; import 'package:flutter_plugin_tools/src/common/file_utils.dart'; import 'package:flutter_plugin_tools/src/common/plugin_utils.dart'; import 'package:flutter_plugin_tools/src/native_test_command.dart'; +import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; import 'package:test/test.dart';