diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index c0bffa0afc126..f54d3c78a15e2 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -7,6 +7,7 @@ import 'dart:io' as io show File, stderr, stdout; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; +import 'package:process/process.dart'; import 'package:process_runner/process_runner.dart'; import 'src/command.dart'; @@ -43,17 +44,34 @@ class _SetStatusCommand { /// A class that runs clang-tidy on all or only the changed files in a git /// repo. class ClangTidy { - /// Given the path to the build commands for a repo and its root, builds - /// an instance of [ClangTidy]. + /// Builds an instance of [ClangTidy] using a repo's [buildCommandPath]. /// - /// `buildCommandsPath` is the path to the build_commands.json file. - /// `repoPath` is the path to the Engine repo. - /// `checksArg` are specific checks for clang-tidy to do. - /// `lintAll` when true indicates that all files should be linted. - /// `outSink` when provided is the destination for normal log messages, which - /// will otherwise go to stdout. - /// `errSink` when provided is the destination for error messages, which - /// will otherwise go to stderr. + /// ## Required + /// - [buildCommandsPath] is the path to the build_commands.json file. + /// + /// ## Optional + /// - [checksArg] are specific checks for clang-tidy to do. + /// + /// If omitted, checks will be determined by the `.clang-tidy` file in the + /// repo. + /// - [lintAll] when true indicates that all files should be linted. + /// + /// ## Optional (Test Overrides) + /// + /// _Most usages of this class will not need to override the following, which + /// are primarily used for testing (i.e. to avoid real interaction with I/O)._ + /// + /// - [outSink] when provided is the destination for normal log messages. + /// + /// If omitted, [io.stdout] will be used. + /// + /// - [errSink] when provided is the destination for error messages. + /// + /// If omitted, [io.stderr] will be used. + /// + /// - [processManager] when provided is delegated to for running processes. + /// + /// If omitted, [LocalProcessManager] will be used. ClangTidy({ required io.File buildCommandsPath, String checksArg = '', @@ -62,6 +80,7 @@ class ClangTidy { bool fix = false, StringSink? outSink, StringSink? errSink, + ProcessManager processManager = const LocalProcessManager(), }) : options = Options( buildCommandsPath: buildCommandsPath, @@ -72,22 +91,26 @@ class ClangTidy { errSink: errSink, ), _outSink = outSink ?? io.stdout, - _errSink = errSink ?? io.stderr; + _errSink = errSink ?? io.stderr, + _processManager = processManager; /// Builds an instance of [ClangTidy] from a command line. ClangTidy.fromCommandLine( List args, { StringSink? outSink, StringSink? errSink, + ProcessManager processManager = const LocalProcessManager(), }) : options = Options.fromCommandLine(args, errSink: errSink), _outSink = outSink ?? io.stdout, - _errSink = errSink ?? io.stderr; + _errSink = errSink ?? io.stderr, + _processManager = processManager; /// The [Options] that specify how this [ClangTidy] operates. final Options options; final StringSink _outSink; final StringSink _errSink; + final ProcessManager _processManager; late final DateTime _startTime; @@ -185,6 +208,7 @@ class ClangTidy { final GitRepo repo = GitRepo( options.repoPath, + processManager: _processManager, verbose: options.verbose, ); if (options.lintHead) { @@ -351,7 +375,10 @@ class ClangTidy { totalJobs, completed, inProgress, pending, failed)); } - final ProcessPool pool = ProcessPool(printReport: reporter); + final ProcessPool pool = ProcessPool( + printReport: reporter, + processRunner: ProcessRunner(processManager: _processManager), + ); await for (final WorkerJob job in pool.startWorkers(jobs)) { pendingJobs.remove(job.name); if (pendingJobs.isNotEmpty && pendingJobs.length <= 3) { diff --git a/tools/clang_tidy/lib/src/git_repo.dart b/tools/clang_tidy/lib/src/git_repo.dart index 49d55a442efc1..746359bb51198 100644 --- a/tools/clang_tidy/lib/src/git_repo.dart +++ b/tools/clang_tidy/lib/src/git_repo.dart @@ -5,6 +5,7 @@ import 'dart:io' as io show Directory, File; import 'package:path/path.dart' as path; +import 'package:process/process.dart'; import 'package:process_runner/process_runner.dart'; /// Utility methods for working with a git repo. @@ -12,7 +13,8 @@ class GitRepo { /// The git repository rooted at `root`. GitRepo(this.root, { this.verbose = false, - }); + ProcessManager processManager = const LocalProcessManager(), + }) : _processManager = processManager; /// Whether to produce verbose log output. final bool verbose; @@ -20,6 +22,9 @@ class GitRepo { /// The root of the git repo. final io.Directory root; + /// The delegate to use for running processes. + final ProcessManager _processManager; + List? _changedFiles; /// Returns a list of all non-deleted files which differ from the nearest @@ -41,6 +46,7 @@ class GitRepo { Future> _getChangedFiles() async { final ProcessRunner processRunner = ProcessRunner( defaultWorkingDirectory: root, + processManager: _processManager, ); await _fetch(processRunner); ProcessRunnerResult mergeBaseResult = await processRunner.runProcess( diff --git a/tools/clang_tidy/pubspec.yaml b/tools/clang_tidy/pubspec.yaml index 46befe2929db6..5e33c8c0db0e5 100644 --- a/tools/clang_tidy/pubspec.yaml +++ b/tools/clang_tidy/pubspec.yaml @@ -20,6 +20,7 @@ dependencies: args: any meta: any path: any + process: any process_runner: any dev_dependencies: diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 13b2e08188ae1..5fcdd984a73e0 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -9,8 +9,62 @@ import 'package:clang_tidy/src/command.dart'; import 'package:clang_tidy/src/options.dart'; import 'package:litetest/litetest.dart'; import 'package:path/path.dart' as path; +import 'package:process/process.dart'; import 'package:process_runner/process_runner.dart'; +import 'process_fakes.dart'; + +/// A test fixture for the `clang-tidy` tool. +final class Fixture { + /// Simulates running the tool with the given [args]. + factory Fixture.fromCommandLine(List args, { + ProcessManager? processManager, + }) { + processManager ??= FakeProcessManager(); + final StringBuffer outBuffer = StringBuffer(); + final StringBuffer errBuffer = StringBuffer(); + return Fixture._(ClangTidy.fromCommandLine( + args, + outSink: outBuffer, + errSink: errBuffer, + processManager: processManager, + ), errBuffer, outBuffer); + } + + /// Simulates running the tool with the given [options]. + factory Fixture.fromOptions(Options options, { + ProcessManager? processManager, + }) { + processManager ??= FakeProcessManager(); + final StringBuffer outBuffer = StringBuffer(); + final StringBuffer errBuffer = StringBuffer(); + return Fixture._(ClangTidy( + buildCommandsPath: options.buildCommandsPath, + lintAll: options.lintAll, + lintHead: options.lintHead, + fix: options.fix, + outSink: outBuffer, + errSink: errBuffer, + processManager: processManager, + ), errBuffer, outBuffer); + } + + Fixture._( + this.tool, + this.errBuffer, + this.outBuffer, + ); + + /// The `clang-tidy` tool. + final ClangTidy tool; + + /// Captured `stdout` from the tool. + final StringBuffer outBuffer; + + /// Captured `stderr` from the tool. + final StringBuffer errBuffer; +} + // Recorded locally from clang-tidy. const String _tidyOutput = ''' /runtime.dart_isolate.o" in /Users/aaclarke/dev/engine/src/out/host_debug exited with code 1 @@ -61,21 +115,12 @@ Future main(List args) async { final String buildCommands = args[0]; test('--help gives help', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( - [ - '--help', - ], - outSink: outBuffer, - errSink: errBuffer, - ); + final Fixture fixture = Fixture.fromCommandLine(['--help']); + final int result = await fixture.tool.run(); - final int result = await clangTidy.run(); - - expect(clangTidy.options.help, isTrue); + expect(fixture.tool.options.help, isTrue); expect(result, equals(0)); - expect(errBuffer.toString(), contains('Usage: ')); + expect(fixture.errBuffer.toString(), contains('Usage: ')); }); test('trimmed clang-tidy output', () { @@ -83,47 +128,36 @@ Future main(List args) async { }); test('Error when --compile-commands and --target-variant are used together', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( + final Fixture fixture = Fixture.fromCommandLine( [ '--compile-commands', '/unused', '--target-variant', 'unused' ], - outSink: outBuffer, - errSink: errBuffer, ); - final int result = await clangTidy.run(); + final int result = await fixture.tool.run(); - expect(clangTidy.options.help, isFalse); expect(result, equals(1)); - expect(errBuffer.toString(), contains( + expect(fixture.errBuffer.toString(), contains( 'ERROR: --compile-commands option cannot be used with --target-variant.', )); }); test('Error when --compile-commands and --src-dir are used together', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( + final Fixture fixture = Fixture.fromCommandLine( [ '--compile-commands', '/unused', '--src-dir', '/unused', ], - outSink: outBuffer, - errSink: errBuffer, ); + final int result = await fixture.tool.run(); - final int result = await clangTidy.run(); - - expect(clangTidy.options.help, isFalse); expect(result, equals(1)); - expect(errBuffer.toString(), contains( + expect(fixture.errBuffer.toString(), contains( 'ERROR: --compile-commands option cannot be used with --src-dir.', )); }); @@ -150,55 +184,38 @@ Future main(List args) async { ], errSink: errBuffer); expect(options.errorMessage, isNotNull); expect(options.shardId, isNull); - print('foo ${options.errorMessage}'); - expect( - options.errorMessage, - contains( - 'Invalid shard-id value', - )); + expect(options.errorMessage, contains('Invalid shard-id value')); }); }); test('Error when --compile-commands path does not exist', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( + final Fixture fixture = Fixture.fromCommandLine( [ '--compile-commands', '/does/not/exist', ], - outSink: outBuffer, - errSink: errBuffer, ); + final int result = await fixture.tool.run(); - final int result = await clangTidy.run(); - - expect(clangTidy.options.help, isFalse); expect(result, equals(1)); - expect(errBuffer.toString().split('\n')[0], hasMatch( + expect(fixture.errBuffer.toString().split('\n')[0], hasMatch( r"ERROR: Build commands path .*/does/not/exist doesn't exist.", )); }); test('Error when --src-dir path does not exist, uses target variant in path', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( + final Fixture fixture = Fixture.fromCommandLine( [ '--src-dir', '/does/not/exist', '--target-variant', 'ios_debug_unopt', ], - outSink: outBuffer, - errSink: errBuffer, ); + final int result = await fixture.tool.run(); - final int result = await clangTidy.run(); - - expect(clangTidy.options.help, isFalse); expect(result, equals(1)); - expect(errBuffer.toString().split('\n')[0], hasMatch( + expect(fixture.errBuffer.toString().split('\n')[0], hasMatch( r'ERROR: Build commands path .*/does/not/exist' r'[/\\]out[/\\]ios_debug_unopt[/\\]compile_commands.json' r" doesn't exist.", @@ -206,77 +223,90 @@ Future main(List args) async { }); test('Error when --lint-all and --lint-head are used together', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( + final Fixture fixture = Fixture.fromCommandLine( [ '--compile-commands', '/unused', '--lint-all', '--lint-head', ], - outSink: outBuffer, - errSink: errBuffer, ); + final int result = await fixture.tool.run(); - final int result = await clangTidy.run(); - - expect(clangTidy.options.help, isFalse); expect(result, equals(1)); - expect(errBuffer.toString(), contains( + expect(fixture.errBuffer.toString(), contains( 'ERROR: At most one of --lint-all and --lint-head can be passed.', )); }); test('lintAll=true checks all files', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy( - buildCommandsPath: io.File(buildCommands), - lintAll: true, - outSink: outBuffer, - errSink: errBuffer, + final Fixture fixture = Fixture.fromOptions( + Options( + buildCommandsPath: io.File(buildCommands), + lintAll: true, + ), ); - final List fileList = await clangTidy.computeFilesOfInterest(); + final List fileList = await fixture.tool.computeFilesOfInterest(); expect(fileList.length, greaterThan(1000)); }); test('lintAll=false does not check all files', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy( - buildCommandsPath: io.File(buildCommands), - outSink: outBuffer, - errSink: errBuffer, + final Fixture fixture = Fixture.fromOptions( + Options( + buildCommandsPath: io.File(buildCommands), + // Intentional: + // ignore: avoid_redundant_argument_values + lintAll: false, + ), + processManager: FakeProcessManager( + onStart: (List command) { + if (command.first == 'git') { + // This just allows git to not actually be called. + return FakeProcess(); + } + return FakeProcessManager.unhandledStart(command); + }, + ), ); - final List fileList = await clangTidy.computeFilesOfInterest(); + final List fileList = await fixture.tool.computeFilesOfInterest(); expect(fileList.length, lessThan(300)); }); test('Sharding', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy( - buildCommandsPath: io.File(buildCommands), - lintAll: true, - outSink: outBuffer, - errSink: errBuffer, + final Fixture fixture = Fixture.fromOptions( + Options( + buildCommandsPath: io.File(buildCommands), + lintAll: true, + ), + processManager: FakeProcessManager( + onStart: (List command) { + if (command.first == 'git') { + // This just allows git to not actually be called. + return FakeProcess(); + } + return FakeProcessManager.unhandledStart(command); + }, + ), ); - Map makeBuildCommandEntry(String filePath) => { - 'directory': '/unused', - 'command': '../../buildtools/mac-x64/clang/bin/clang $filePath', - 'file': filePath, - }; + + Map makeBuildCommandEntry(String filePath) { + return { + 'directory': '/unused', + 'command': '../../buildtools/mac-x64/clang/bin/clang $filePath', + 'file': filePath, + }; + } + final List filePaths = [ for (int i = 0; i < 10; ++i) '/path/to/a/source_file_$i.cc' ]; - final List buildCommandsData = + final List> buildCommandsData = filePaths.map((String e) => makeBuildCommandEntry(e)).toList(); - final List shardBuildCommandsData = + final List> shardBuildCommandsData = filePaths.sublist(6).map((String e) => makeBuildCommandEntry(e)).toList(); { - final List commands = await clangTidy.getLintCommandsForFiles( + final List commands = await fixture.tool.getLintCommandsForFiles( buildCommandsData, filePaths.map((String e) => io.File(e)).toList(), >[shardBuildCommandsData], @@ -296,10 +326,10 @@ Future main(List args) async { expect(commandFilePaths.contains('/path/to/a/source_file_9.cc'), false); } { - final List commands = await clangTidy.getLintCommandsForFiles( + final List commands = await fixture.tool.getLintCommandsForFiles( buildCommandsData, filePaths.map((String e) => io.File(e)).toList(), - >[shardBuildCommandsData], + >>[shardBuildCommandsData], 1, ); @@ -319,14 +349,13 @@ Future main(List args) async { }); test('No Commands are produced when no files changed', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy( - buildCommandsPath: io.File(buildCommands), - lintAll: true, - outSink: outBuffer, - errSink: errBuffer, + final Fixture fixture = Fixture.fromOptions( + Options( + buildCommandsPath: io.File(buildCommands), + lintAll: true, + ), ); + const String filePath = '/path/to/a/source_file.cc'; final List buildCommandsData = >[ { @@ -335,7 +364,7 @@ Future main(List args) async { 'file': filePath, }, ]; - final List commands = await clangTidy.getLintCommandsForFiles( + final List commands = await fixture.tool.getLintCommandsForFiles( buildCommandsData, [], >[], @@ -346,13 +375,11 @@ Future main(List args) async { }); test('A Command is produced when a file is changed', () async { - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy( - buildCommandsPath: io.File(buildCommands), - lintAll: true, - outSink: outBuffer, - errSink: errBuffer, + final Fixture fixture = Fixture.fromOptions( + Options( + buildCommandsPath: io.File(buildCommands), + lintAll: true, + ), ); // This file needs to exist, and be UTF8 line-parsable. @@ -364,7 +391,7 @@ Future main(List args) async { 'file': filePath, }, ]; - final List commands = await clangTidy.getLintCommandsForFiles( + final List commands = await fixture.tool.getLintCommandsForFiles( buildCommandsData, [io.File(filePath)], >[], diff --git a/tools/clang_tidy/test/process_fakes.dart b/tools/clang_tidy/test/process_fakes.dart new file mode 100644 index 0000000000000..7bc5fe5778e1f --- /dev/null +++ b/tools/clang_tidy/test/process_fakes.dart @@ -0,0 +1,117 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:convert'; +import 'dart:io' as io; + +import 'package:process/process.dart'; + +/// A fake implementation of [ProcessManager] that allows control for testing. +final class FakeProcessManager implements ProcessManager { + FakeProcessManager({ + io.ProcessResult Function(List command) onRun = unhandledRun, + io.Process Function(List command) onStart = unhandledStart, + }) : _onRun = onRun, _onStart = onStart; + + static io.ProcessResult unhandledRun(List command) { + throw UnsupportedError('Unhandled run: ${command.join(' ')}'); + } + + static io.Process unhandledStart(List command) { + throw UnsupportedError('Unhandled start: ${command.join(' ')}'); + } + + final io.ProcessResult Function(List command) _onRun; + final io.Process Function(List command) _onStart; + + @override + bool canRun(Object? executable, {String? workingDirectory}) => true; + + @override + bool killPid(int pid, [io.ProcessSignal signal = io.ProcessSignal.sigterm]) => true; + + @override + Future run( + List command, { + String? workingDirectory, + Map? environment, + bool includeParentEnvironment = true, + bool runInShell = false, + Encoding stdoutEncoding = io.systemEncoding, + Encoding stderrEncoding = io.systemEncoding, + }) async { + return runSync( + command, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + } + + @override + io.ProcessResult runSync( + List command, { + String? workingDirectory, + Map? environment, + bool includeParentEnvironment = true, + bool runInShell = false, + Encoding stdoutEncoding = io.systemEncoding, + Encoding stderrEncoding = io.systemEncoding, + }) { + return _onRun(command.map((Object o) => '$o').toList()); + } + + @override + Future start( + List command, { + String? workingDirectory, + Map? environment, + bool includeParentEnvironment = true, + bool runInShell = false, + io.ProcessStartMode mode = io.ProcessStartMode.normal, + }) async { + return _onStart(command.map((Object o) => '$o').toList()); + } +} + +/// An incomplete fake of [io.Process] that allows control for testing. +final class FakeProcess implements io.Process { + /// Creates a fake process that returns the given [exitCode] and out/err. + FakeProcess({ + int exitCode = 0, + String stdout = '', + String stderr = '', + }) : _exitCode = exitCode, + _stdout = stdout, + _stderr = stderr; + + final int _exitCode; + final String _stdout; + final String _stderr; + + @override + Future get exitCode async => _exitCode; + + @override + bool kill([io.ProcessSignal signal = io.ProcessSignal.sigterm]) => true; + + @override + int get pid => 0; + + @override + Stream> get stderr { + return Stream>.fromIterable(>[io.systemEncoding.encoder.convert(_stderr)]); + } + + @override + io.IOSink get stdin => throw UnimplementedError(); + + @override + Stream> get stdout { + return Stream>.fromIterable(>[io.systemEncoding.encoder.convert(_stdout)]); + } +}