[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 flutter/flutter#117905
flutter · Apr 18, 2024 · bb82cb7 · bb82cb7
1 parent b0e6d7f
commit bb82cb7
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 56 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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>[]);
-
-        package.pubspecFile.writeAsStringSync('''
+      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>[]);
+
+            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;
-        });
-
-        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.'),
-          ]),
-        );
+            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 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>[]);