DEV/flutter_flutter
1
0
Fork 0
mirror of https://github.com/flutter/flutter.git synced 2026-02-20 02:29:02 +08:00
Code Issues Packages Projects Releases Wiki Activity

Do not run real processes in clang_tidy_test.dart (flutter/engine#45748)

Browse Source 
Partial work towards https://github.com/flutter/flutter/issues/133190.

Some context, from @zanderso on chat:

@matanlurey:

> For
[`clang_tidy_test.dart`](c020936303/tools/clang_tidy/test/clang_tidy_test.dart),
what is your vision there? I remember we ran into problems with it
actually executing `clang.run()` in too many of the test cases, but I
don't remember (or seem to have written down) what we decided to do 🙂

@zanderso:

> The existing tests are calling `computeFilesOfInterest` and
`getLintCommandsForFiles` and inspecting the results for positive tests
instead of hitting a real clang-tidy invocation. Tests of command line
flags that call `clangTidy.run()` are also testing cases that
short-circuit or fail before making a real invocation. Making it harder
to do a real invocation from a unit test probably requires plumbing a
fake-able ProcessRunner or ProcessManager object through to the
ClangTidy constructor.

---

All this PR does is allow providing a `ProcessManager` (defaults to
`LocalProcessManager`, i.e. `dart:io`) throughout the tool. Then, for
tests, I've implemented a _very_ minimal fake of both `ProcessManager`
and `Process` (it would be nice to share code w/ say, `flutter_tool`,
but lots of work/overhead for very little surface area).

After this CL, no tests in `clang_tidy_test.dart` actually run a
process. This should unblock adding new features (i.e. original intent
of https://github.com/flutter/flutter/issues/133190) without causing
headaches for others/CI?
This commit is contained in:
Matan Lurey 2023-09-13 09:49:49 -07:00 committed by GitHub
parent 47da5d4f07
commit 367f454a40
5 changed files with 303 additions and 125 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
engine/src/flutter/tools/clang_tidy/lib/clang_tidy.dart
View File

@ -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<String> 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) {

8
engine/src/flutter/tools/clang_tidy/lib/src/git_repo.dart
View File

@ -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<io.File>? _changedFiles;
/// Returns a list of all non-deleted files which differ from the nearest
@ -41,6 +46,7 @@ class GitRepo {
Future<List<io.File>> _getChangedFiles() async {
final ProcessRunner processRunner = ProcessRunner(
defaultWorkingDirectory: root,
processManager: _processManager,
);
await _fetch(processRunner);
ProcessRunnerResult mergeBaseResult = await processRunner.runProcess(

1
engine/src/flutter/tools/clang_tidy/pubspec.yaml
View File

@ -20,6 +20,7 @@ dependencies:
args: any
meta: any
path: any
process: any
process_runner: any
dev_dependencies:

249
engine/src/flutter/tools/clang_tidy/test/clang_tidy_test.dart
View File

@ -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<String> 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<int> main(List<String> 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(
<String>[
'--help',
],
outSink: outBuffer,
errSink: errBuffer,
);
final Fixture fixture = Fixture.fromCommandLine(<String>['--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<int> main(List<String> 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(
<String>[
'--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(
<String>[
'--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<int> main(List<String> 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(
<String>[
'--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(
<String>[
'--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<int> main(List<String> 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(
<String>[
'--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<io.File> fileList = await clangTidy.computeFilesOfInterest();
final List<io.File> 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<String> command) {
if (command.first == 'git') {
// This just allows git to not actually be called.
return FakeProcess();
}
return FakeProcessManager.unhandledStart(command);
},
),
);
final List<io.File> fileList = await clangTidy.computeFilesOfInterest();
final List<io.File> 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<String> command) {
if (command.first == 'git') {
// This just allows git to not actually be called.
return FakeProcess();
}
return FakeProcessManager.unhandledStart(command);
},
),
);
Map<String, dynamic> makeBuildCommandEntry(String filePath) => <String, dynamic>{
'directory': '/unused',
'command': '../../buildtools/mac-x64/clang/bin/clang $filePath',
'file': filePath,
};
Map<String, String> makeBuildCommandEntry(String filePath) {
return <String, String>{
'directory': '/unused',
'command': '../../buildtools/mac-x64/clang/bin/clang $filePath',
'file': filePath,
};
}
final List<String> filePaths = <String>[
for (int i = 0; i < 10; ++i) '/path/to/a/source_file_$i.cc'
];
final List<dynamic> buildCommandsData =
final List<Map<String, String>> buildCommandsData =
filePaths.map((String e) => makeBuildCommandEntry(e)).toList();
final List<dynamic> shardBuildCommandsData =
final List<Map<String, String>> shardBuildCommandsData =
filePaths.sublist(6).map((String e) => makeBuildCommandEntry(e)).toList();
{
final List<Command> commands = await clangTidy.getLintCommandsForFiles(
final List<Command> commands = await fixture.tool.getLintCommandsForFiles(
buildCommandsData,
filePaths.map((String e) => io.File(e)).toList(),
<List<dynamic>>[shardBuildCommandsData],
@ -296,10 +326,10 @@ Future<int> main(List<String> args) async {
expect(commandFilePaths.contains('/path/to/a/source_file_9.cc'), false);
}
{
final List<Command> commands = await clangTidy.getLintCommandsForFiles(
final List<Command> commands = await fixture.tool.getLintCommandsForFiles(
buildCommandsData,
filePaths.map((String e) => io.File(e)).toList(),
<List<dynamic>>[shardBuildCommandsData],
<List<Map<String, String>>>[shardBuildCommandsData],
1,
);
@ -319,14 +349,13 @@ Future<int> main(List<String> 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<dynamic> buildCommandsData = <Map<String, dynamic>>[
<String, dynamic>{
@ -335,7 +364,7 @@ Future<int> main(List<String> args) async {
'file': filePath,
},
];
final List<Command> commands = await clangTidy.getLintCommandsForFiles(
final List<Command> commands = await fixture.tool.getLintCommandsForFiles(
buildCommandsData,
<io.File>[],
<List<dynamic>>[],
@ -346,13 +375,11 @@ Future<int> main(List<String> 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<int> main(List<String> args) async {
'file': filePath,
},
];
final List<Command> commands = await clangTidy.getLintCommandsForFiles(
final List<Command> commands = await fixture.tool.getLintCommandsForFiles(
buildCommandsData,
<io.File>[io.File(filePath)],
<List<dynamic>>[],

117
engine/src/flutter/tools/clang_tidy/test/process_fakes.dart Normal file
View File

@ -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<String> command) onRun = unhandledRun,
io.Process Function(List<String> command) onStart = unhandledStart,
}) : _onRun = onRun, _onStart = onStart;
static io.ProcessResult unhandledRun(List<String> command) {
throw UnsupportedError('Unhandled run: ${command.join(' ')}');
}
static io.Process unhandledStart(List<String> command) {
throw UnsupportedError('Unhandled start: ${command.join(' ')}');
}
final io.ProcessResult Function(List<String> command) _onRun;
final io.Process Function(List<String> command) _onStart;
@override
bool canRun(Object? executable, {String? workingDirectory}) => true;
@override
bool killPid(int pid, [io.ProcessSignal signal = io.ProcessSignal.sigterm]) => true;
@override
Future<io.ProcessResult> run(
List<Object> command, {
String? workingDirectory,
Map<String, String>? 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<Object> command, {
String? workingDirectory,
Map<String, String>? 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<io.Process> start(
List<Object> command, {
String? workingDirectory,
Map<String, String>? 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<int> get exitCode async => _exitCode;
@override
bool kill([io.ProcessSignal signal = io.ProcessSignal.sigterm]) => true;
@override
int get pid => 0;
@override
Stream<List<int>> get stderr {
return Stream<List<int>>.fromIterable(<List<int>>[io.systemEncoding.encoder.convert(_stderr)]);
}
@override
io.IOSink get stdin => throw UnimplementedError();
@override
Stream<List<int>> get stdout {
return Stream<List<int>>.fromIterable(<List<int>>[io.systemEncoding.encoder.convert(_stdout)]);
}
}