-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[flutter_plugin_tool] Add custom-test command #5058
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'package:file/file.dart'; | ||
| import 'package:platform/platform.dart'; | ||
|
|
||
| import 'common/package_looping_command.dart'; | ||
| import 'common/process_runner.dart'; | ||
| import 'common/repository_package.dart'; | ||
|
|
||
| const String _scriptName = 'run_tests.dart'; | ||
| const String _legacyScriptName = 'run_tests.sh'; | ||
|
|
||
| /// A command to run custom, package-local tests on packages. | ||
| /// | ||
| /// This is an escape hatch for adding tests that this tooling doesn't support. | ||
| /// It should be used sparingly; prefer instead to add functionality to this | ||
| /// tooling to eliminate the need for bespoke tests. | ||
| class CustomTestCommand extends PackageLoopingCommand { | ||
| /// Creates a custom test command instance. | ||
| CustomTestCommand( | ||
| Directory packagesDir, { | ||
| ProcessRunner processRunner = const ProcessRunner(), | ||
| Platform platform = const LocalPlatform(), | ||
| }) : super(packagesDir, processRunner: processRunner, platform: platform); | ||
|
|
||
| @override | ||
| final String name = 'custom-test'; | ||
|
|
||
| @override | ||
| final String description = 'Runs package-specific custom tests defined in ' | ||
| 'a package\'s tool/$_scriptName file.\n\n' | ||
| 'This command requires "dart" to be in your path.'; | ||
|
|
||
| @override | ||
| Future<PackageResult> runForPackage(RepositoryPackage package) async { | ||
| final File script = | ||
| package.directory.childDirectory('tool').childFile(_scriptName); | ||
| final File legacyScript = package.directory.childFile(_legacyScriptName); | ||
| String? customSkipReason; | ||
| bool ranTests = false; | ||
|
|
||
| // Run the custom Dart script if presest. | ||
| if (script.existsSync()) { | ||
| final int exitCode = await processRunner.runAndStream( | ||
| 'dart', <String>['run', 'tool/$_scriptName'], | ||
| workingDir: package.directory); | ||
| if (exitCode != 0) { | ||
| return PackageResult.fail(); | ||
| } | ||
| ranTests = true; | ||
| } | ||
|
|
||
| // Run the legacy script if present. | ||
| if (legacyScript.existsSync()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will make it so we run the dart script and the sh script. That will cause certain tests to get executed twice since the shell script delegates to the dart script. We should execute one or the other. We could juggle things around in Pigeon so that the correct way is to execute the dart script and it will delegate to a shell script test that haven't been migrated (if it isn't windows). We could even make run_tests.sh just execute the dart script and there is another bash script the dart script delegates to for unmigrated tests.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's already how it works. It had to work that way in order to support pigeon's tests, which were partially implemented in one and partially implemented in the other.
This code was supposed to prevent that. If Pigeon's test structure changed after that in a way that causes it to double-run tests, that's a Pigeon test bug.
I don't have any opinion on how Pigeon manages the Dart vs shell split in the interim period until all tests are migrated, but it's orthogonal to this PR since the behavior isn't changing.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fair enough.
I'm just trying to think what the fix is though given this logic and making sure we have a path forward. The easiest thing for pigeon would be to rename the script 'run_tests.sh' to 'shell_tests.sh' and make run_tests.dart execute shell_tests.sh. If pigeon isn't using run_tests.sh then probably no one is so we can get rid of this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not clear on what the problem actually is. What is the flow where the code I linked to fails to avoid duplicate tests?
Four packages currently use |
||
| if (platform.isWindows) { | ||
| customSkipReason = '$_legacyScriptName is not supported on Windows. ' | ||
| 'Please migrate to $_scriptName.'; | ||
| } else { | ||
| final int exitCode = await processRunner.runAndStream( | ||
| legacyScript.path, <String>[], | ||
| workingDir: package.directory); | ||
| if (exitCode != 0) { | ||
| return PackageResult.fail(); | ||
| } | ||
| ranTests = true; | ||
| } | ||
| } | ||
|
|
||
| if (!ranTests) { | ||
| return PackageResult.skip(customSkipReason ?? 'No custom tests'); | ||
| } | ||
|
|
||
| return PackageResult.success(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,281 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:io' as io; | ||
|
|
||
| import 'package:args/command_runner.dart'; | ||
| import 'package:file/file.dart'; | ||
| import 'package:file/memory.dart'; | ||
| import 'package:flutter_plugin_tools/src/common/core.dart'; | ||
| import 'package:flutter_plugin_tools/src/custom_test_command.dart'; | ||
| import 'package:test/test.dart'; | ||
|
|
||
| import 'mocks.dart'; | ||
| import 'util.dart'; | ||
|
|
||
| void main() { | ||
| late FileSystem fileSystem; | ||
| late MockPlatform mockPlatform; | ||
| late Directory packagesDir; | ||
| late RecordingProcessRunner processRunner; | ||
| late CommandRunner<void> runner; | ||
|
|
||
| group('posix', () { | ||
| setUp(() { | ||
| fileSystem = MemoryFileSystem(); | ||
| mockPlatform = MockPlatform(); | ||
| packagesDir = createPackagesDirectory(fileSystem: fileSystem); | ||
| processRunner = RecordingProcessRunner(); | ||
| final CustomTestCommand analyzeCommand = CustomTestCommand( | ||
| packagesDir, | ||
| processRunner: processRunner, | ||
| platform: mockPlatform, | ||
| ); | ||
|
|
||
| runner = CommandRunner<void>( | ||
| 'custom_test_command', 'Test for custom_test_command'); | ||
| runner.addCommand(analyzeCommand); | ||
| }); | ||
|
|
||
| test('runs both new and legacy when both are present', () async { | ||
| final Directory package = | ||
| createFakePlugin('a_package', packagesDir, extraFiles: <String>[ | ||
| 'tool/run_tests.dart', | ||
| 'run_tests.sh', | ||
| ]); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect( | ||
| processRunner.recordedCalls, | ||
| containsAll(<ProcessCall>[ | ||
| ProcessCall(package.childFile('run_tests.sh').path, | ||
| const <String>[], package.path), | ||
| ProcessCall('dart', const <String>['run', 'tool/run_tests.dart'], | ||
| package.path), | ||
| ])); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('Ran for 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('runs when only new is present', () async { | ||
| final Directory package = createFakePlugin('a_package', packagesDir, | ||
| extraFiles: <String>['tool/run_tests.dart']); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect( | ||
| processRunner.recordedCalls, | ||
| containsAll(<ProcessCall>[ | ||
| ProcessCall('dart', const <String>['run', 'tool/run_tests.dart'], | ||
| package.path), | ||
| ])); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('Ran for 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('runs when only legacy is present', () async { | ||
| final Directory package = createFakePlugin('a_package', packagesDir, | ||
| extraFiles: <String>['run_tests.sh']); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect( | ||
| processRunner.recordedCalls, | ||
| containsAll(<ProcessCall>[ | ||
| ProcessCall(package.childFile('run_tests.sh').path, | ||
| const <String>[], package.path), | ||
| ])); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('Ran for 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('skips when neither is present', () async { | ||
| createFakePlugin('a_package', packagesDir); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect(processRunner.recordedCalls, isEmpty); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('Skipped 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('fails if new fails', () async { | ||
| createFakePlugin('a_package', packagesDir, extraFiles: <String>[ | ||
| 'tool/run_tests.dart', | ||
| 'run_tests.sh', | ||
| ]); | ||
|
|
||
| processRunner.mockProcessesForExecutable['dart'] = <io.Process>[ | ||
| MockProcess(exitCode: 1), | ||
| ]; | ||
|
|
||
| Error? commandError; | ||
| final List<String> output = await runCapturingPrint( | ||
| runner, <String>['custom-test'], errorHandler: (Error e) { | ||
| commandError = e; | ||
| }); | ||
|
|
||
| expect(commandError, isA<ToolExit>()); | ||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('The following packages had errors:'), | ||
| contains('a_package') | ||
| ])); | ||
| }); | ||
|
|
||
| test('fails if legacy fails', () async { | ||
| final Directory package = | ||
| createFakePlugin('a_package', packagesDir, extraFiles: <String>[ | ||
| 'tool/run_tests.dart', | ||
| 'run_tests.sh', | ||
| ]); | ||
|
|
||
| processRunner.mockProcessesForExecutable[ | ||
| package.childFile('run_tests.sh').path] = <io.Process>[ | ||
| MockProcess(exitCode: 1), | ||
| ]; | ||
|
|
||
| Error? commandError; | ||
| final List<String> output = await runCapturingPrint( | ||
| runner, <String>['custom-test'], errorHandler: (Error e) { | ||
| commandError = e; | ||
| }); | ||
|
|
||
| expect(commandError, isA<ToolExit>()); | ||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('The following packages had errors:'), | ||
| contains('a_package') | ||
| ])); | ||
| }); | ||
| }); | ||
|
|
||
| group('Windows', () { | ||
| setUp(() { | ||
| fileSystem = MemoryFileSystem(style: FileSystemStyle.windows); | ||
| mockPlatform = MockPlatform(isWindows: true); | ||
| packagesDir = createPackagesDirectory(fileSystem: fileSystem); | ||
| processRunner = RecordingProcessRunner(); | ||
| final CustomTestCommand analyzeCommand = CustomTestCommand( | ||
| packagesDir, | ||
| processRunner: processRunner, | ||
| platform: mockPlatform, | ||
| ); | ||
|
|
||
| runner = CommandRunner<void>( | ||
| 'custom_test_command', 'Test for custom_test_command'); | ||
| runner.addCommand(analyzeCommand); | ||
| }); | ||
|
|
||
| test('runs new and skips old when both are present', () async { | ||
| final Directory package = | ||
| createFakePlugin('a_package', packagesDir, extraFiles: <String>[ | ||
| 'tool/run_tests.dart', | ||
| 'run_tests.sh', | ||
| ]); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect( | ||
| processRunner.recordedCalls, | ||
| containsAll(<ProcessCall>[ | ||
| ProcessCall('dart', const <String>['run', 'tool/run_tests.dart'], | ||
| package.path), | ||
| ])); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('Ran for 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('runs when only new is present', () async { | ||
| final Directory package = createFakePlugin('a_package', packagesDir, | ||
| extraFiles: <String>['tool/run_tests.dart']); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect( | ||
| processRunner.recordedCalls, | ||
| containsAll(<ProcessCall>[ | ||
| ProcessCall('dart', const <String>['run', 'tool/run_tests.dart'], | ||
| package.path), | ||
| ])); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('Ran for 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('skips package when only legacy is present', () async { | ||
| createFakePlugin('a_package', packagesDir, | ||
| extraFiles: <String>['run_tests.sh']); | ||
|
|
||
| final List<String> output = | ||
| await runCapturingPrint(runner, <String>['custom-test']); | ||
|
|
||
| expect(processRunner.recordedCalls, isEmpty); | ||
|
|
||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('run_tests.sh is not supported on Windows'), | ||
| contains('Skipped 1 package(s)'), | ||
| ])); | ||
| }); | ||
|
|
||
| test('fails if new fails', () async { | ||
| createFakePlugin('a_package', packagesDir, extraFiles: <String>[ | ||
| 'tool/run_tests.dart', | ||
| 'run_tests.sh', | ||
| ]); | ||
|
|
||
| processRunner.mockProcessesForExecutable['dart'] = <io.Process>[ | ||
| MockProcess(exitCode: 1), | ||
| ]; | ||
|
|
||
| Error? commandError; | ||
| final List<String> output = await runCapturingPrint( | ||
| runner, <String>['custom-test'], errorHandler: (Error e) { | ||
| commandError = e; | ||
| }); | ||
|
|
||
| expect(commandError, isA<ToolExit>()); | ||
| expect( | ||
| output, | ||
| containsAllInOrder(<Matcher>[ | ||
| contains('The following packages had errors:'), | ||
| contains('a_package') | ||
| ])); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pigeon has this in
bin; I'll move it when I switch flutter/packages to this new command. Dart package layout docs say thatbinis for package-client-facing commands, and scripts intended for package developers (the call out test scripts as an explicit example) belong intool.