From 6775f9cc1a811a7e238d5497fb5a03c897052f36 Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Mon, 1 Apr 2024 10:50:14 -0400 Subject: [PATCH 1/7] Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies --- .../tool/lib/src/pubspec_check_command.dart | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 378dfc65caa..37174510dae 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -356,7 +356,7 @@ class PubspecCheckCommand extends PackageLoopingCommand { 'a topic. Add "$topicName" to the "topics" section.'; } } - + // Validates topic names according to https://dart.dev/tools/pub/pubspec#topics final RegExp expectedTopicFormat = RegExp(r'^[a-z](?:-?[a-z0-9]+)*$'); final Iterable invalidTopics = topics.where((String topic) => @@ -542,17 +542,25 @@ class PubspecCheckCommand extends PackageLoopingCommand { // there are any that aren't allowed. String? _checkDependencies(Pubspec pubspec) { final Set badDependencies = {}; + // Shipped dependencies. for (final Map dependencies - in >[ - pubspec.dependencies, - pubspec.devDependencies - ]) { + in >[pubspec.dependencies]) { dependencies.forEach((String name, Dependency dependency) { if (!_shouldAllowDependency(name, dependency)) { badDependencies.add(name); } }); } + // Dev dependencies + for (final Map dependencies + in >[pubspec.devDependencies]) { + dependencies.forEach((String name, Dependency dependency) { + if (!_shouldAllowDevDependency(name, dependency)) { + badDependencies.add(name); + } + }); + } + if (badDependencies.isEmpty) { return null; } @@ -563,7 +571,23 @@ class PubspecCheckCommand extends PackageLoopingCommand { } // Checks whether a given dependency is allowed. + // Defaults to false. bool _shouldAllowDependency(String name, Dependency dependency) { + const List disallowedSdkDependencies = ['integration_test']; + if (dependency is SdkDependency && + disallowedSdkDependencies.contains(name)) { + return false; + } + return _shouldAllowBaselineDependency(name, dependency); + } + + // Checks whether a given dependency is allowed as a dev dependency. + bool _shouldAllowDevDependency(String name, Dependency dependency) { + return _shouldAllowBaselineDependency(name, dependency); + } + + // Shared enforcement between dev and non dev dependencies. + bool _shouldAllowBaselineDependency(String name, Dependency dependency) { if (dependency is PathDependency || dependency is SdkDependency) { return true; } From f2685ab3655c8ea4ca5e1442383f0d18eaf2270c Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Mon, 1 Apr 2024 11:05:18 -0400 Subject: [PATCH 2/7] Add test for dev/nondev dependency check --- .../tool/test/pubspec_check_command_test.dart | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index 8e99d720f57..62406f2c5e7 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -1734,6 +1734,39 @@ ${_topicsSection()} ]), ); }); + + test('fails when integration_test is used in non dev dependency', + () async { + final RepositoryPackage package = + createFakePackage('a_package', packagesDir, examples: []); + + package.pubspecFile.writeAsStringSync(''' +${_headerSection('a_package')} +${_environmentSection()} +${_dependenciesSection(['integration_test: \n sdk: flutter'])} +${_devDependenciesSection()} +${_topicsSection()} +'''); + + Error? commandError; + final List output = await runCapturingPrint(runner, [ + 'pubspec-check', + ], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains( + 'The following unexpected non-local dependencies were found:\n' + ' integration_test\n' + 'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies ' + 'for more information and next steps.'), + ]), + ); + }); }); }); From cc4e650ccaab622eaf5ec30270c459b53e35e03a Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Mon, 1 Apr 2024 11:21:33 -0400 Subject: [PATCH 3/7] Reduce code complexity --- .../tool/lib/src/pubspec_check_command.dart | 37 ++++++------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 37174510dae..0f348d1836f 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -544,22 +544,24 @@ class PubspecCheckCommand extends PackageLoopingCommand { final Set badDependencies = {}; // Shipped dependencies. for (final Map dependencies - in >[pubspec.dependencies]) { + in >[ + pubspec.dependencies, + pubspec.devDependencies + ]) { dependencies.forEach((String name, Dependency dependency) { if (!_shouldAllowDependency(name, dependency)) { badDependencies.add(name); } }); } - // Dev dependencies - for (final Map dependencies - in >[pubspec.devDependencies]) { - dependencies.forEach((String name, Dependency dependency) { - if (!_shouldAllowDevDependency(name, dependency)) { - badDependencies.add(name); - } - }); - } + + // Ensure that dev-only dependencies aren't in `dependencies`. + const List devOnlyDependencies = ['integration_test']; + pubspec.dependencies.forEach((String name, Dependency dependency) { + if (devOnlyDependencies.contains(name)) { + badDependencies.add(name); + } + }); if (badDependencies.isEmpty) { return null; @@ -573,21 +575,6 @@ class PubspecCheckCommand extends PackageLoopingCommand { // Checks whether a given dependency is allowed. // Defaults to false. bool _shouldAllowDependency(String name, Dependency dependency) { - const List disallowedSdkDependencies = ['integration_test']; - if (dependency is SdkDependency && - disallowedSdkDependencies.contains(name)) { - return false; - } - return _shouldAllowBaselineDependency(name, dependency); - } - - // Checks whether a given dependency is allowed as a dev dependency. - bool _shouldAllowDevDependency(String name, Dependency dependency) { - return _shouldAllowBaselineDependency(name, dependency); - } - - // Shared enforcement between dev and non dev dependencies. - bool _shouldAllowBaselineDependency(String name, Dependency dependency) { if (dependency is PathDependency || dependency is SdkDependency) { return true; } From c0c3860838750c934cef832114964fd165a8ea69 Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Tue, 2 Apr 2024 11:38:33 -0400 Subject: [PATCH 4/7] Add exception for shared_test_plugin_code --- .../tool/lib/src/pubspec_check_command.dart | 18 ++++++++--- .../tool/test/pubspec_check_command_test.dart | 31 +++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 0f348d1836f..8a846c7a828 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -557,11 +557,19 @@ class PubspecCheckCommand extends PackageLoopingCommand { // Ensure that dev-only dependencies aren't in `dependencies`. const List devOnlyDependencies = ['integration_test']; - pubspec.dependencies.forEach((String name, Dependency dependency) { - if (devOnlyDependencies.contains(name)) { - badDependencies.add(name); - } - }); + // pigeon/platform_tests/shared_test_plugin_code is allowed to violate + // the dev only dependencies rule beause pidgeon has generated tests that + // are intended to ship to customers. + if (pubspec.name == 'shared_test_plugin_code') { + printWarning( + ' Skipping dev-only dependencies check for ${pubspec.name}'); + } else { + pubspec.dependencies.forEach((String name, Dependency dependency) { + if (devOnlyDependencies.contains(name)) { + badDependencies.add(name); + } + }); + } if (badDependencies.isEmpty) { return null; diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index 62406f2c5e7..37bb3479eec 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -1767,6 +1767,37 @@ ${_topicsSection()} ]), ); }); + + test('passes when integration_test is used in shared_test_plugin_code', + () async { + final RepositoryPackage package = createFakePackage( + 'shared_test_plugin_code', packagesDir, + examples: []); + + package.pubspecFile.writeAsStringSync(''' +${_headerSection('shared_test_plugin_code')} +${_environmentSection()} +${_dependenciesSection(['integration_test: \n sdk: flutter'])} +${_devDependenciesSection()} +${_topicsSection()} +'''); + + Error? commandError; + final List output = await runCapturingPrint(runner, [ + 'pubspec-check', + ], errorHandler: (Error e) { + commandError = e; + }); + + expect( + output, + containsAllInOrder([ + contains('Running for shared_test_plugin_code...'), + contains( + 'Skipping dev-only dependencies check for shared_test_plugin_code'), + ]), + ); + }); }); }); From 1b2a8846c9bc9d13c74fb3afb6783e7083b1dd3f Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Tue, 2 Apr 2024 11:46:40 -0400 Subject: [PATCH 5/7] formatting --- script/tool/test/pubspec_check_command_test.dart | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index 37bb3479eec..c6e6681e3fa 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -1782,12 +1782,9 @@ ${_devDependenciesSection()} ${_topicsSection()} '''); - Error? commandError; final List output = await runCapturingPrint(runner, [ 'pubspec-check', - ], errorHandler: (Error e) { - commandError = e; - }); + ]); expect( output, From 5c903535d285f3f2131b295d1fb5dd01b9692ae2 Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Wed, 3 Apr 2024 10:34:18 -0400 Subject: [PATCH 6/7] Allow violations of dev_dependencies for non published packages --- script/tool/lib/src/pubspec_check_command.dart | 10 +++------- script/tool/test/pubspec_check_command_test.dart | 14 ++++++-------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 8a846c7a828..46aa7066efe 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -557,13 +557,9 @@ class PubspecCheckCommand extends PackageLoopingCommand { // Ensure that dev-only dependencies aren't in `dependencies`. const List devOnlyDependencies = ['integration_test']; - // pigeon/platform_tests/shared_test_plugin_code is allowed to violate - // the dev only dependencies rule beause pidgeon has generated tests that - // are intended to ship to customers. - if (pubspec.name == 'shared_test_plugin_code') { - printWarning( - ' Skipping dev-only dependencies check for ${pubspec.name}'); - } else { + // non published packages like pidgeon 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); diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index c6e6681e3fa..8427ea3dcbb 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -1768,14 +1768,13 @@ ${_topicsSection()} ); }); - test('passes when integration_test is used in shared_test_plugin_code', + test('passes when integration_test is used in non published package', () async { - final RepositoryPackage package = createFakePackage( - 'shared_test_plugin_code', packagesDir, - examples: []); + final RepositoryPackage package = + createFakePackage('a_package', packagesDir, examples: []); package.pubspecFile.writeAsStringSync(''' -${_headerSection('shared_test_plugin_code')} +${_headerSection('a_package', publishable: false)} ${_environmentSection()} ${_dependenciesSection(['integration_test: \n sdk: flutter'])} ${_devDependenciesSection()} @@ -1789,9 +1788,8 @@ ${_topicsSection()} expect( output, containsAllInOrder([ - contains('Running for shared_test_plugin_code...'), - contains( - 'Skipping dev-only dependencies check for shared_test_plugin_code'), + contains('Running for a_package...'), + contains('Ran for'), ]), ); }); From e93f7a602871ae7621e5e8ff812dec8a8b0c3e01 Mon Sep 17 00:00:00 2001 From: Reid Baker Date: Wed, 3 Apr 2024 10:55:28 -0400 Subject: [PATCH 7/7] Spelling --- script/tool/lib/src/pubspec_check_command.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 46aa7066efe..008dc2df3b6 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -557,7 +557,7 @@ class PubspecCheckCommand extends PackageLoopingCommand { // Ensure that dev-only dependencies aren't in `dependencies`. const List devOnlyDependencies = ['integration_test']; - // non published packages like pidgeon subpackages are allowed to violate + // Non-published packages like pidgeon subpackages are allowed to violate // the dev only dependencies rule. if (pubspec.publishTo != 'none') { pubspec.dependencies.forEach((String name, Dependency dependency) {