Skip to content

[ci] Add more dev dependency checks, and fix errors #6563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 18, 2024
Merged

[ci] Add more dev dependency checks, and fix errors #6563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions packages/camera/camera_avfoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.9.15+3

* Moves `pigeon` to `dev_dependencies`.

## 0.9.15+2

* Converts camera query to Pigeon.
Expand Down
4 changes: 2 additions & 2 deletions packages/camera/camera_avfoundation/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -29,6 +28,7 @@ dev_dependencies:
flutter_test:
sdk: flutter
mockito: 5.4.4
pigeon: ^18.0.0

topics:
- camera
42 changes: 31 additions & 11 deletions script/tool/lib/src/pubspec_check_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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>>[
Expand All @@ -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.
Expand Down
100 changes: 57 additions & 43 deletions script/tool/test/pubspec_check_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.'),
]),
);
});
Expand Down Expand Up @@ -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.'),
]),
);
});
Expand Down Expand Up @@ -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',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched the structure to a loop-constructed test set where each test only checks one package, because it's easier to ensure that everything is actually being covered (e.g., that we don't add a package to the list of dependencies, but forget to add it to the error message, so that it looks like it's being tested but actually isn't).

() 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>[]);
Expand Down