Skip to content

Commit

Permalink
Do not run real processes in clang_tidy_test.dart (flutter#45748)
Browse files Browse the repository at this point in the history
Partial work towards flutter/flutter#133190.

Some context, from @zanderso on chat:

@matanlurey:

> For
[`clang_tidy_test.dart`](https://github.com/flutter/engine/blob/c020936303cb0646cd10593e6e427d8c5aa6ce52/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 flutter/flutter#133190) without causing
headaches for others/CI?
  • Loading branch information
matanlurey authored Sep 13, 2023
1 parent 3b6d0ea commit b71b366
Show file tree
Hide file tree
Showing 5 changed files with 303 additions and 125 deletions.
53 changes: 40 additions & 13 deletions tools/clang_tidy/lib/clang_tidy.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 = '',
Expand All @@ -62,6 +80,7 @@ class ClangTidy {
bool fix = false,
StringSink? outSink,
StringSink? errSink,
ProcessManager processManager = const LocalProcessManager(),
}) :
options = Options(
buildCommandsPath: buildCommandsPath,
Expand All @@ -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;

Expand Down Expand Up @@ -185,6 +208,7 @@ class ClangTidy {

final GitRepo repo = GitRepo(
options.repoPath,
processManager: _processManager,
verbose: options.verbose,
);
if (options.lintHead) {
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 7 additions & 1 deletion tools/clang_tidy/lib/src/git_repo.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,26 @@
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.
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;

/// 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
Expand All @@ -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(
Expand Down
1 change: 1 addition & 0 deletions tools/clang_tidy/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ dependencies:
args: any
meta: any
path: any
process: any
process_runner: any

dev_dependencies:
Expand Down
Loading

0 comments on commit b71b366

Please sign in to comment.