From 471b83c526405707bc3a5e037b3806b55d83aba1 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 12 Oct 2021 11:00:47 -0400 Subject: [PATCH 1/2] [flutter_plugin_tools] Fix pubspec-check on Windows The repository check always failed when run on Windows, because the relative path generated to check the end of the URL was using local filesystem style for separators, but URLs always use POSIX separators. --- script/tool/CHANGELOG.md | 1 + .../tool/lib/src/pubspec_check_command.dart | 2 +- .../tool/test/pubspec_check_command_test.dart | 394 ++++++++++-------- 3 files changed, 222 insertions(+), 175 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index a5263ba03965..bc62ac4e44d0 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -9,6 +9,7 @@ - Added flags to `version-check` to allow overriding the platform interface major version change restriction. - Improved error handling and error messages in CHANGELOG version checks. +- Fixed `pubspec-check` on Windows. ## 0.7.1 diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index fec0dcef9ac7..bdae1b0ffc9e 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -165,7 +165,7 @@ class PubspecCheckCommand extends PackageLoopingCommand { errorMessages.add('Missing "repository"'); } else { final String relativePackagePath = - path.relative(package.path, from: packagesDir.parent.path); + getRelativePosixPath(package.directory, from: packagesDir.parent); if (!pubspec.repository!.path.endsWith(relativePackagePath)) { errorMessages .add('The "repository" link should end with the package path.'); diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index 948136993d18..03eb574af478 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -12,59 +12,33 @@ import 'package:test/test.dart'; import 'mocks.dart'; import 'util.dart'; -void main() { - group('test pubspec_check_command', () { - late CommandRunner runner; - late RecordingProcessRunner processRunner; - late FileSystem fileSystem; - late MockPlatform mockPlatform; - late Directory packagesDir; - - setUp(() { - fileSystem = MemoryFileSystem(); - mockPlatform = MockPlatform(); - packagesDir = fileSystem.currentDirectory.childDirectory('packages'); - createPackagesDirectory(parentDir: packagesDir.parent); - processRunner = RecordingProcessRunner(); - final PubspecCheckCommand command = PubspecCheckCommand( - packagesDir, - processRunner: processRunner, - platform: mockPlatform, - ); - - runner = CommandRunner( - 'pubspec_check_command', 'Test for pubspec_check_command'); - runner.addCommand(command); - }); - - /// Returns the top section of a pubspec.yaml for a package named [name], - /// for either a flutter/packages or flutter/plugins package depending on - /// the values of [isPlugin]. - /// - /// By default it will create a header that includes all of the expected - /// values, elements can be changed via arguments to create incorrect - /// entries. - /// - /// If [includeRepository] is true, by default the path in the link will - /// be "packages/[name]"; a different "packages"-relative path can be - /// provided with [repositoryPackagesDirRelativePath]. - String headerSection( - String name, { - bool isPlugin = false, - bool includeRepository = true, - String? repositoryPackagesDirRelativePath, - bool includeHomepage = false, - bool includeIssueTracker = true, - bool publishable = true, - }) { - final String repositoryPath = repositoryPackagesDirRelativePath ?? name; - final String repoLink = 'https://github.com/flutter/' - '${isPlugin ? 'plugins' : 'packages'}/tree/master/' - 'packages/$repositoryPath'; - final String issueTrackerLink = - 'https://github.com/flutter/flutter/issues?' - 'q=is%3Aissue+is%3Aopen+label%3A%22p%3A+$name%22'; - return ''' +/// Returns the top section of a pubspec.yaml for a package named [name], +/// for either a flutter/packages or flutter/plugins package depending on +/// the values of [isPlugin]. +/// +/// By default it will create a header that includes all of the expected +/// values, elements can be changed via arguments to create incorrect +/// entries. +/// +/// If [includeRepository] is true, by default the path in the link will +/// be "packages/[name]"; a different "packages"-relative path can be +/// provided with [repositoryPackagesDirRelativePath]. +String _headerSection( + String name, { + bool isPlugin = false, + bool includeRepository = true, + String? repositoryPackagesDirRelativePath, + bool includeHomepage = false, + bool includeIssueTracker = true, + bool publishable = true, +}) { + final String repositoryPath = repositoryPackagesDirRelativePath ?? name; + final String repoLink = 'https://github.com/flutter/' + '${isPlugin ? 'plugins' : 'packages'}/tree/master/' + 'packages/$repositoryPath'; + final String issueTrackerLink = 'https://github.com/flutter/flutter/issues?' + 'q=is%3Aissue+is%3Aopen+label%3A%22p%3A+$name%22'; + return ''' name: $name ${includeRepository ? 'repository: $repoLink' : ''} ${includeHomepage ? 'homepage: $repoLink' : ''} @@ -72,64 +46,89 @@ ${includeIssueTracker ? 'issue_tracker: $issueTrackerLink' : ''} version: 1.0.0 ${publishable ? '' : 'publish_to: \'none\''} '''; - } +} - String environmentSection() { - return ''' +String _environmentSection() { + return ''' environment: sdk: ">=2.12.0 <3.0.0" flutter: ">=2.0.0" '''; - } +} - String flutterSection({ - bool isPlugin = false, - String? implementedPackage, - }) { - final String pluginEntry = ''' +String _flutterSection({ + bool isPlugin = false, + String? implementedPackage, +}) { + final String pluginEntry = ''' plugin: ${implementedPackage == null ? '' : ' implements: $implementedPackage'} platforms: '''; - return ''' + return ''' flutter: ${isPlugin ? pluginEntry : ''} '''; - } +} - String dependenciesSection() { - return ''' +String _dependenciesSection() { + return ''' dependencies: flutter: sdk: flutter '''; - } +} - String devDependenciesSection() { - return ''' +String _devDependenciesSection() { + return ''' dev_dependencies: flutter_test: sdk: flutter '''; - } +} - String falseSecretsSection() { - return ''' +String _falseSecretsSection() { + return ''' false_secrets: - /lib/main.dart '''; - } +} + +void main() { + group('test pubspec_check_command', () { + late CommandRunner runner; + late RecordingProcessRunner processRunner; + late FileSystem fileSystem; + late MockPlatform mockPlatform; + late Directory packagesDir; + + setUp(() { + fileSystem = MemoryFileSystem(); + mockPlatform = MockPlatform(); + packagesDir = fileSystem.currentDirectory.childDirectory('packages'); + createPackagesDirectory(parentDir: packagesDir.parent); + processRunner = RecordingProcessRunner(); + final PubspecCheckCommand command = PubspecCheckCommand( + packagesDir, + processRunner: processRunner, + platform: mockPlatform, + ); + + runner = CommandRunner( + 'pubspec_check_command', 'Test for pubspec_check_command'); + runner.addCommand(command); + }); test('passes for a plugin following conventions', () async { final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} -${falseSecretsSection()} +${_headerSection('plugin', isPlugin: true)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} +${_falseSecretsSection()} '''); final List output = await runCapturingPrint(runner, [ @@ -150,12 +149,12 @@ ${falseSecretsSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin')} -${environmentSection()} -${dependenciesSection()} -${devDependenciesSection()} -${flutterSection()} -${falseSecretsSection()} +${_headerSection('plugin')} +${_environmentSection()} +${_dependenciesSection()} +${_devDependenciesSection()} +${_flutterSection()} +${_falseSecretsSection()} '''); final List output = await runCapturingPrint(runner, [ @@ -177,9 +176,9 @@ ${falseSecretsSection()} packageDirectory.createSync(recursive: true); packageDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('package')} -${environmentSection()} -${dependenciesSection()} +${_headerSection('package')} +${_environmentSection()} +${_dependenciesSection()} '''); final List output = await runCapturingPrint(runner, [ @@ -199,11 +198,11 @@ ${dependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true, includeHomepage: true)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true, includeHomepage: true)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -226,11 +225,11 @@ ${devDependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true, includeRepository: false)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true, includeRepository: false)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -252,11 +251,11 @@ ${devDependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true, includeHomepage: true, includeRepository: false)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true, includeHomepage: true, includeRepository: false)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -279,11 +278,11 @@ ${devDependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true, repositoryPackagesDirRelativePath: 'different_plugin')} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true, repositoryPackagesDirRelativePath: 'different_plugin')} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -305,11 +304,11 @@ ${devDependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true, includeIssueTracker: false)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true, includeIssueTracker: false)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -331,11 +330,11 @@ ${devDependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true)} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} -${environmentSection()} +${_headerSection('plugin', isPlugin: true)} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} +${_environmentSection()} '''); Error? commandError; @@ -358,11 +357,11 @@ ${environmentSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true)} -${flutterSection(isPlugin: true)} -${environmentSection()} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true)} +${_flutterSection(isPlugin: true)} +${_environmentSection()} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -385,11 +384,11 @@ ${devDependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${devDependenciesSection()} -${dependenciesSection()} +${_headerSection('plugin', isPlugin: true)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_devDependenciesSection()} +${_dependenciesSection()} '''); Error? commandError; @@ -412,11 +411,11 @@ ${dependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true)} -${environmentSection()} -${devDependenciesSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} +${_headerSection('plugin', isPlugin: true)} +${_environmentSection()} +${_devDependenciesSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} '''); Error? commandError; @@ -439,12 +438,12 @@ ${dependenciesSection()} final Directory pluginDirectory = createFakePlugin('plugin', packagesDir); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin', isPlugin: true)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${falseSecretsSection()} -${devDependenciesSection()} +${_headerSection('plugin', isPlugin: true)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_falseSecretsSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -469,11 +468,11 @@ ${devDependenciesSection()} 'plugin_a_foo', packagesDir.childDirectory('plugin_a')); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin_a_foo', isPlugin: true)} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin_a_foo', isPlugin: true)} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -497,11 +496,11 @@ ${devDependenciesSection()} 'plugin_a_foo', packagesDir.childDirectory('plugin_a')); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin_a_foo', isPlugin: true)} -${environmentSection()} -${flutterSection(isPlugin: true, implementedPackage: 'plugin_a_foo')} -${dependenciesSection()} -${devDependenciesSection()} +${_headerSection('plugin_a_foo', isPlugin: true)} +${_environmentSection()} +${_flutterSection(isPlugin: true, implementedPackage: 'plugin_a_foo')} +${_dependenciesSection()} +${_devDependenciesSection()} '''); Error? commandError; @@ -525,15 +524,15 @@ ${devDependenciesSection()} 'plugin_a_foo', packagesDir.childDirectory('plugin_a')); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection( +${_headerSection( 'plugin_a_foo', isPlugin: true, repositoryPackagesDirRelativePath: 'plugin_a/plugin_a_foo', )} -${environmentSection()} -${flutterSection(isPlugin: true, implementedPackage: 'plugin_a')} -${dependenciesSection()} -${devDependenciesSection()} +${_environmentSection()} +${_flutterSection(isPlugin: true, implementedPackage: 'plugin_a')} +${_dependenciesSection()} +${_devDependenciesSection()} '''); final List output = @@ -553,15 +552,15 @@ ${devDependenciesSection()} createFakePlugin('plugin_a', packagesDir.childDirectory('plugin_a')); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection( +${_headerSection( 'plugin_a', isPlugin: true, repositoryPackagesDirRelativePath: 'plugin_a/plugin_a', )} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); final List output = @@ -583,16 +582,16 @@ ${devDependenciesSection()} packagesDir.childDirectory('plugin_a')); pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection( +${_headerSection( 'plugin_a_platform_interface', isPlugin: true, repositoryPackagesDirRelativePath: 'plugin_a/plugin_a_platform_interface', )} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); final List output = @@ -614,11 +613,11 @@ ${devDependenciesSection()} // Environment section is in the wrong location. // Missing 'implements'. pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection('plugin_a_foo', isPlugin: true, publishable: false)} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} -${environmentSection()} +${_headerSection('plugin_a_foo', isPlugin: true, publishable: false)} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} +${_environmentSection()} '''); Error? commandError; @@ -644,17 +643,17 @@ ${environmentSection()} // Missing metadata that is only useful for published packages, such as // repository and issue tracker. pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' -${headerSection( +${_headerSection( 'plugin', isPlugin: true, publishable: false, includeRepository: false, includeIssueTracker: false, )} -${environmentSection()} -${flutterSection(isPlugin: true)} -${dependenciesSection()} -${devDependenciesSection()} +${_environmentSection()} +${_flutterSection(isPlugin: true)} +${_dependenciesSection()} +${_devDependenciesSection()} '''); final List output = @@ -669,4 +668,51 @@ ${devDependenciesSection()} ); }); }); + + group('test pubspec_check_command on Windows', () { + late CommandRunner runner; + late RecordingProcessRunner processRunner; + late FileSystem fileSystem; + late MockPlatform mockPlatform; + late Directory packagesDir; + + setUp(() { + fileSystem = MemoryFileSystem(style: FileSystemStyle.windows); + mockPlatform = MockPlatform(isWindows: true); + packagesDir = fileSystem.currentDirectory.childDirectory('packages'); + createPackagesDirectory(parentDir: packagesDir.parent); + processRunner = RecordingProcessRunner(); + final PubspecCheckCommand command = PubspecCheckCommand( + packagesDir, + processRunner: processRunner, + platform: mockPlatform, + ); + + runner = CommandRunner( + 'pubspec_check_command', 'Test for pubspec_check_command'); + runner.addCommand(command); + }); + + test('repository check works', () async { + final Directory packageDirectory = + createFakePackage('package', packagesDir); + + packageDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${_headerSection('package')} +${_environmentSection()} +${_dependenciesSection()} +'''); + + final List output = + await runCapturingPrint(runner, ['pubspec-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for package...'), + contains('No issues found!'), + ]), + ); + }); + }); } From 8499de2d3bedf0b7137c81378a6c5d7a2cca74b1 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 20 Oct 2021 10:05:29 -0400 Subject: [PATCH 2/2] Review comments --- script/tool/test/pubspec_check_command_test.dart | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart index ee4a1033f73f..ba943903b6c5 100644 --- a/script/tool/test/pubspec_check_command_test.dart +++ b/script/tool/test/pubspec_check_command_test.dart @@ -34,9 +34,16 @@ String _headerSection( String? description, }) { final String repositoryPath = repositoryPackagesDirRelativePath ?? name; - final String repoLink = 'https://github.com/flutter/' - '${isPlugin ? 'plugins' : 'packages'}/tree/master/' - 'packages/$repositoryPath'; + final List repoLinkPathComponents = [ + 'flutter', + if (isPlugin) 'plugins' else 'packages', + 'tree', + 'master', + 'packages', + repositoryPath, + ]; + final String repoLink = + 'https://github.com/' + repoLinkPathComponents.join('/'); final String issueTrackerLink = 'https://github.com/flutter/flutter/issues?' 'q=is%3Aissue+is%3Aopen+label%3A%22p%3A+$name%22'; description ??= 'A test package for validating that the pubspec.yaml '