From bb82cb79b92c41a6dea8e28f60fbf818a9d82dc8 Mon Sep 17 00:00:00 2001 From: stuartmorgan <stuartmorgan@google.com> Date: Thu, 18 Apr 2024 17:43:04 -0400 Subject: [PATCH] [ci] Add more dev dependency checks, and fix errors (#6563) - Adds `build_runner`, `mockito`, and `pigeon` to the list of dependencies that we expect to only be in dev_dependencies. - Fixes the error message for violations; I had missed in initial review that the errorr message was the same as the one for non-repo-local depndencies, making it misleading about what the problem was and how to fix it. (Also fixed the indentation, which had always been wrong.) - Fixes `camera_avfoundation`, which had a violation introduced in my recent PR to start the Pigeon conversion. See https://github.com/flutter/flutter/issues/117905 --- .../camera/camera_avfoundation/CHANGELOG.md | 4 + .../camera/camera_avfoundation/pubspec.yaml | 4 +- .../tool/lib/src/pubspec_check_command.dart | 42 +++++--- .../tool/test/pubspec_check_command_test.dart | 96 +++++++++++-------- 4 files changed, 92 insertions(+), 54 deletions(-) diff --git a/packages/camera/camera_avfoundation/CHANGELOG.md b/packages/camera/camera_avfoundation/CHANGELOG.md index 19b43a6885..265cad31e8 100644 --- a/packages/camera/camera_avfoundation/CHANGELOG.md +++ b/packages/camera/camera_avfoundation/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.9.15+3 + +* Moves `pigeon` to `dev_dependencies`. + ## 0.9.15+2 * Converts camera query to Pigeon. diff --git a/packages/camera/camera_avfoundation/pubspec.yaml b/packages/camera/camera_avfoundation/pubspec.yaml index a599285232..8dd2b6663a 100644 --- a/packages/camera/camera_avfoundation/pubspec.yaml +++ b/packages/camera/camera_avfoundation/pubspec.yaml @@ -2,7 +2,7 @@ name: camera_avfoundation description: iOS implementation of the camera plugin. repository: https://github.com/flutter/packages/tree/main/packages/camera/camera_avfoundation issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22 -version: 0.9.15+2 +version: 0.9.15+3 environment: sdk: ^3.2.3 @@ -20,7 +20,6 @@ dependencies: camera_platform_interface: ^2.7.0 flutter: sdk: flutter - pigeon: ^18.0.0 stream_transform: ^2.0.0 dev_dependencies: @@ -29,6 +28,7 @@ dev_dependencies: flutter_test: sdk: flutter mockito: 5.4.4 + pigeon: ^18.0.0 topics: - camera diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 08360aae18..90f9324fd2 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -200,7 +200,10 @@ class PubspecCheckCommand extends PackageLoopingCommand { final String? dependenciesError = _checkDependencies(pubspec); if (dependenciesError != null) { - printError('$indentation$dependenciesError'); + printError(dependenciesError + .split('\n') + .map((String line) => '$indentation$line') + .join('\n')); passing = false; } @@ -542,6 +545,7 @@ class PubspecCheckCommand extends PackageLoopingCommand { // there are any that aren't allowed. String? _checkDependencies(Pubspec pubspec) { final Set<String> badDependencies = <String>{}; + final Set<String> misplacedDevDependencies = <String>{}; // Shipped dependencies. for (final Map<String, Dependency> dependencies in <Map<String, Dependency>>[ @@ -556,24 +560,40 @@ class PubspecCheckCommand extends PackageLoopingCommand { } // Ensure that dev-only dependencies aren't in `dependencies`. - const Set<String> devOnlyDependencies = <String>{'integration_test', 'test', 'flutter_test'}; - // Non-published packages like pidgeon subpackages are allowed to violate + const Set<String> devOnlyDependencies = <String>{ + 'build_runner', + 'integration_test', + 'flutter_test', + 'mockito', + 'pigeon', + 'test', + }; + // Non-published packages like pigeon subpackages are allowed to violate // the dev only dependencies rule. if (pubspec.publishTo != 'none') { pubspec.dependencies.forEach((String name, Dependency dependency) { if (devOnlyDependencies.contains(name)) { - badDependencies.add(name); + misplacedDevDependencies.add(name); } }); } - if (badDependencies.isEmpty) { - return null; - } - return 'The following unexpected non-local dependencies were found:\n' - '${badDependencies.map((String name) => ' $name').join('\n')}\n' - 'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies ' - 'for more information and next steps.'; + final List<String> errors = <String>[ + if (badDependencies.isNotEmpty) + ''' +The following unexpected non-local dependencies were found: +${badDependencies.map((String name) => ' $name').join('\n')} +Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies +for more information and next steps. +''', + if (misplacedDevDependencies.isNotEmpty) + ''' +The following dev dependencies were found in the dependencies section: +${misplacedDevDependencies.map((String name) => ' $name').join('\n')} +Please move them to dev_dependencies. +''', + ]; + return errors.isEmpty ? null : errors.join('\n\n'); } // Checks whether a given dependency is allowed. diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index 7e575a3041..00255f4173 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -1611,10 +1611,10 @@ ${_topicsSection()} output, containsAllInOrder(<Matcher>[ contains( - 'The following unexpected non-local dependencies were found:\n' - ' bad_dependency\n' - 'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies ' - 'for more information and next steps.'), + ' The following unexpected non-local dependencies were found:\n' + ' bad_dependency\n' + ' Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies\n' + ' for more information and next steps.'), ]), ); }); @@ -1643,10 +1643,10 @@ ${_topicsSection()} output, containsAllInOrder(<Matcher>[ contains( - 'The following unexpected non-local dependencies were found:\n' - ' bad_dependency\n' - 'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies ' - 'for more information and next steps.'), + ' The following unexpected non-local dependencies were found:\n' + ' bad_dependency\n' + ' Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies\n' + ' for more information and next steps.'), ]), ); }); @@ -1727,54 +1727,68 @@ ${_topicsSection()} output, containsAllInOrder(<Matcher>[ contains( - 'The following unexpected non-local dependencies were found:\n' - ' allow_pinned\n' - 'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies ' - 'for more information and next steps.'), + ' The following unexpected non-local dependencies were found:\n' + ' allow_pinned\n' + ' Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies\n' + ' for more information and next steps.'), ]), ); }); - test('fails when integration_test, flutter_test or test are used in non dev dependency', - () async { - final RepositoryPackage package = - createFakePackage('a_package', packagesDir, examples: <String>[]); + group('dev dependencies', () { + const List<String> packages = <String>[ + 'build_runner', + 'integration_test', + 'flutter_test', + 'mockito', + 'pigeon', + 'test', + ]; + for (final String dependency in packages) { + test('fails when $dependency is used in non dev dependency', + () async { + final RepositoryPackage package = createFakePackage( + 'a_package', packagesDir, + examples: <String>[]); - package.pubspecFile.writeAsStringSync(''' + final String version = + dependency == 'integration_test' || dependency == 'flutter_test' + ? '{ sdk: flutter }' + : '1.0.0'; + package.pubspecFile.writeAsStringSync(''' ${_headerSection('a_package')} ${_environmentSection()} ${_dependenciesSection(<String>[ - 'integration_test: \n sdk: flutter', - 'flutter_test: \n sdk: flutter', - 'test: 1.0.0' - ])} + '$dependency: $version', + ])} ${_devDependenciesSection()} ${_topicsSection()} '''); - Error? commandError; - final List<String> output = await runCapturingPrint(runner, <String>[ - 'pubspec-check', - ], errorHandler: (Error e) { - commandError = e; - }); + Error? commandError; + final List<String> output = + await runCapturingPrint(runner, <String>[ + 'pubspec-check', + ], errorHandler: (Error e) { + commandError = e; + }); - expect(commandError, isA<ToolExit>()); - expect( - output, - containsAllInOrder(<Matcher>[ - contains( - 'The following unexpected non-local dependencies were found:\n' - ' test\n' - ' integration_test\n' - ' flutter_test\n' - 'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies ' - 'for more information and next steps.'), - ]), - ); + expect(commandError, isA<ToolExit>()); + expect( + output, + containsAllInOrder(<Matcher>[ + contains( + ' The following dev dependencies were found in the dependencies section:\n' + ' $dependency\n' + ' Please move them to dev_dependencies.'), + ]), + ); + }); + } }); - test('passes when integration_test or flutter_test are used in non published package', + test( + 'passes when integration_test or flutter_test are used in non published package', () async { final RepositoryPackage package = createFakePackage('a_package', packagesDir, examples: <String>[]);