Make health testable

.github/workflows/health.yaml

@@ -78,8 +78,7 @@ on:
         type: boolean
         required: false
       use-flutter:
-        description: >-
-          Whether to setup Flutter in this workflow.
+        description: Whether to setup Flutter in this workflow.
         default: false
         required: false
         type: boolean

.github/workflows/health_base.yaml

@@ -45,8 +45,7 @@ on:
         type: boolean
         required: false
       use-flutter:
-        description: >-
-          Whether to setup Flutter in this workflow.
+        description: Whether to setup Flutter in this workflow.
         default: false
         required: false
         type: boolean
@@ -118,7 +117,7 @@ jobs:
         if: ${{ '$action_state' == 1 }}
 
       - name: Upload coverage to Coveralls
-        if: ${{ inputs.upload_coverage }}
+        if: ${{ inputs.upload_coverage && inputs.check == 'coverage' }}
         uses: coverallsapp/github-action@3dfc5567390f6fa9267c0ee9c251e4c8c3f18949
         with:
           format: lcov

pkgs/firehose/.pubignore

@@ -0,0 +1 @@
+test_data/

pkgs/firehose/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.6.0
+
+- Make the health workflow testable with golden tests.
+
 ## 0.5.2
 
 - Also run health workflows on bot PRs.

pkgs/firehose/analysis_options.yaml

@@ -1 +1,5 @@
 include: package:dart_flutter_team_lints/analysis_options.yaml
+
+analyzer:
+  exclude:
+    - test_data/*

pkgs/firehose/bin/health.dart

@@ -5,6 +5,7 @@
 import 'dart:io';
 
 import 'package:args/args.dart';
+import 'package:firehose/src/github.dart';
 import 'package:firehose/src/health/health.dart';
 
 void main(List<String> arguments) async {
@@ -37,6 +38,12 @@ void main(List<String> arguments) async {
     throw ArgumentError('The checks for which warnings are displayed and the '
         'checks which lead to failure must be disjoint.');
   }
-  await Health(Directory.current, check, warnOn, failOn, coverageWeb)
-      .healthCheck();
+  await Health(
+    Directory.current,
+    check,
+    warnOn,
+    failOn,
+    coverageWeb,
+    GithubApi(),
+  ).healthCheck();
 }

pkgs/firehose/lib/firehose.dart

@@ -91,7 +91,7 @@ Saving existing comment id $existingCommentId to file ${idFile.path}''');
   }
 
   Future<VerificationResults> verify(GithubApi github) async {
-    var repo = Repository();
+    var repo = Repository(directory);
     var packages = repo.locatePackages();
 
     var pub = Pub();

pkgs/firehose/lib/src/github.dart

@@ -19,16 +19,13 @@ class GithubApi {
 
   static Map<String, String> get _env => Platform.environment;
 
-  /// When true, details of any RPC error are printed to the console.
-  final bool verbose;
-
-  GithubApi({this.verbose = false, RepositorySlug? repoSlug, int? issueNumber})
+  GithubApi({RepositorySlug? repoSlug, int? issueNumber})
       : _repoSlug = repoSlug,
         _issueNumber = issueNumber;
 
   final http.Client _client = DelayedClient(const Duration(milliseconds: 50));
 
-  late GitHub github = githubAuthToken != null
+  late final GitHub _github = githubAuthToken != null
       ? GitHub(
           auth: Authentication.withToken(githubAuthToken),
           client: _client,
@@ -95,7 +92,7 @@ class GithubApi {
     required String user,
     String? searchTerm,
   }) async {
-    final matchingComment = await github.issues
+    final matchingComment = await _github.issues
         .listCommentsByIssue(repoSlug!, issueNumber!)
         .map<IssueComment?>((comment) => comment)
         .firstWhere(
@@ -110,38 +107,41 @@ class GithubApi {
     return matchingComment?.id;
   }
 
-  Future<List<GitFile>> listFilesForPR() async => await github.pullRequests
-      .listFiles(repoSlug!, issueNumber!)
-      .map((prFile) =>
-          GitFile(prFile.filename!, FileStatus.fromString(prFile.status!)))
-      .toList();
+  Future<List<GitFile>> listFilesForPR(Directory directory) async =>
+      await _github.pullRequests
+          .listFiles(repoSlug!, issueNumber!)
+          .map((prFile) => GitFile(
+                prFile.filename!,
+                FileStatus.fromString(prFile.status!),
+                directory,
+              ))
+          .toList();
 
   /// Write a notice message to the github log.
   void notice({required String message}) {
     print('::notice ::$message');
   }
 
   Future<String> pullrequestBody() async {
-    final pullRequest = await github.pullRequests.get(repoSlug!, issueNumber!);
+    final pullRequest = await _github.pullRequests.get(repoSlug!, issueNumber!);
     return pullRequest.body ?? '';
   }
 
-  void close() => github.dispose();
+  void close() => _github.dispose();
 }
 
 class GitFile {
   final String filename;
   final FileStatus status;
+  final Directory directory;
 
   bool isInPackage(Package package) {
-    print('Check if $relativePath is in ${package.directory.path}');
-    return path.isWithin(package.directory.path, relativePath);
+    return path.isWithin(package.directory.path, pathInRepository);
   }
 
-  GitFile(this.filename, this.status);
+  String get pathInRepository => path.join(directory.path, filename);
 
-  String get relativePath =>
-      path.relative(filename, from: Directory.current.path);
+  GitFile(this.filename, this.status, this.directory);
 
   @override
   String toString() => '$filename: $status';

pkgs/firehose/lib/src/health/changelog.dart

@@ -8,23 +8,27 @@ import 'package:path/path.dart' as path;
 
 import '../github.dart';
 import '../repo.dart';
-import '../utils.dart';
 
 Future<Map<Package, List<GitFile>>> packagesWithoutChangelog(
-    GithubApi github) async {
-  final repo = Repository();
+  GithubApi github,
+  Directory directory,
+) async {
+  final repo = Repository(directory);
   final packages = repo.locatePackages();
 
-  final files = await github.listFilesForPR();
+  final files = await github.listFilesForPR(directory);
 
-  var packagesWithoutChangedChangelog =
-      collectPackagesWithoutChangelogChanges(packages, files);
+  var packagesWithoutChangedChangelog = collectPackagesWithoutChangelogChanges(
+    packages,
+    files,
+    directory,
+  );
 
   print('Collecting files without license headers in those packages:');
   var packagesWithChanges = <Package, List<GitFile>>{};
   for (final file in files) {
     for (final package in packagesWithoutChangedChangelog) {
-      if (fileNeedsEntryInChangelog(package, file.relativePath)) {
+      if (fileNeedsEntryInChangelog(package, file.filename, directory)) {
         print(file);
         packagesWithChanges.update(
           package,
@@ -40,21 +44,24 @@ Done, found ${packagesWithChanges.length} packages with a need for a changelog.'
 }
 
 List<Package> collectPackagesWithoutChangelogChanges(
-    List<Package> packages, List<GitFile> files) {
+  List<Package> packages,
+  List<GitFile> files,
+  Directory directory,
+) {
   print('Collecting packages without changed changelogs:');
-  final packagesWithoutChangedChangelog = packages
-      .where((package) => package.changelog.exists)
-      .where((package) => !files
-          .map((e) => e.relativePath)
-          .contains(package.changelog.file.relativePath))
-      .toList();
+  final packagesWithoutChangedChangelog =
+      packages.where((package) => package.changelog.exists).where((package) {
+    return !files
+        .map((e) => e.pathInRepository)
+        .contains(package.changelog.file.path);
+  }).toList();
   print('Done, found ${packagesWithoutChangedChangelog.length} packages.');
   return packagesWithoutChangedChangelog;
 }
 
-bool fileNeedsEntryInChangelog(Package package, String file) {
+bool fileNeedsEntryInChangelog(Package package, String file, Directory d) {
   final directoryPath = package.directory.path;
-  final directory = path.relative(directoryPath, from: Directory.current.path);
+  final directory = path.relative(directoryPath, from: d.path);
   final isInPackage = path.isWithin(directory, file);
   final isInLib = path.isWithin(path.join(directory, 'lib'), file);
   final isInBin = path.isWithin(path.join(directory, 'bin'), file);

pkgs/firehose/lib/src/health/coverage.dart

@@ -5,6 +5,7 @@
 
 import 'dart:io';
 
+import 'package:collection/collection.dart';
 import 'package:path/path.dart' as path;
 
 import '../github.dart';
@@ -14,38 +15,38 @@ import 'lcov.dart';
 
 class Coverage {
   final bool coverageWeb;
+  final Directory directory;
 
-  Coverage(this.coverageWeb);
+  Coverage(this.coverageWeb, this.directory);
 
-  Future<CoverageResult> compareCoverages(GithubApi github) async {
-    var files = await github.listFilesForPR();
-    var basePath = '../base_repo/';
+  Future<CoverageResult> compareCoverages(
+      GithubApi github, Directory base) async {
+    var files = await github.listFilesForPR(directory);
 
-    return compareCoveragesFor(files, basePath);
+    return compareCoveragesFor(files, base);
   }
 
-  CoverageResult compareCoveragesFor(List<GitFile> files, String basePath) {
-    var repository = Repository();
+  CoverageResult compareCoveragesFor(List<GitFile> files, Directory base) {
+    var repository = Repository(directory);
     var packages = repository.locatePackages();
-    print('Found packages $packages at ${Directory.current}');
+    print('Found packages $packages at $directory');
 
     var filesOfInterest = files
         .where((file) => path.extension(file.filename) == '.dart')
         .where((file) => file.status != FileStatus.removed)
-        .where((file) => isInSomePackage(packages, file.relativePath))
-        .where((file) => isNotATest(packages, file.relativePath))
+        .where((file) => isInSomePackage(packages, file.filename))
+        .where((file) => isNotATest(packages, file.filename))
         .toList();
     print('The files of interest are $filesOfInterest');
 
-    var base = Directory(basePath);
-
     var baseRepository = Repository(base);
     var basePackages = baseRepository.locatePackages();
     print('Found packages $basePackages at $base');
 
     var changedPackages = packages
         .where((package) =>
             filesOfInterest.any((file) => file.isInPackage(package)))
+        .sortedBy((package) => package.name)
         .toList();
 
     print('The packages of interest are $changedPackages');
@@ -58,14 +59,15 @@ class Coverage {
           .where((element) => element.name == package.name)
           .firstOrNull;
       final oldCoverages = getCoverage(basePackage);
-      var filePaths = filesOfInterest
+      var filenames = filesOfInterest
           .where((file) => file.isInPackage(package))
-          .map((file) => file.relativePath);
-      for (var filePath in filePaths) {
-        var oldCoverage = oldCoverages[filePath];
-        var newCoverage = newCoverages[filePath];
-        print('Compage coverage for $filePath: $oldCoverage vs $newCoverage');
-        coverageResult[filePath] = Change(
+          .map((file) => file.filename)
+          .sortedBy((filename) => filename);
+      for (var filename in filenames) {
+        var oldCoverage = oldCoverages[filename];
+        var newCoverage = newCoverages[filename];
+        print('Compage coverage for $filename: $oldCoverage vs $newCoverage');
+        coverageResult[filename] = Change(
           oldCoverage: oldCoverage,
           newCoverage: newCoverage,
         );
@@ -75,14 +77,16 @@ class Coverage {
   }
 
   bool isNotATest(List<Package> packages, String file) {
-    return packages.every((package) =>
-        !path.isWithin(path.join(package.directory.path, 'test'), file));
+    return packages.every((package) => !path.isWithin(
+        path.join(package.directory.path, 'test'),
+        path.join(directory.path, file)));
   }
 
-  bool isInSomePackage(List<Package> packages, String file) {
-    return packages
-        .any((package) => path.isWithin(package.directory.path, file));
-  }
+  bool isInSomePackage(List<Package> packages, String file) =>
+      packages.any((package) => path.isWithin(
+            package.directory.path,
+            path.join(directory.path, file),
+          ));
 
   Map<String, double> getCoverage(Package? package) {
     if (package != null) {
@@ -97,7 +101,7 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path
           workingDirectory: package.directory.path,
         );
         if (coverageWeb) {
-          print('Get test coverage for web');
+          print('Run tests with coverage for web');
           var resultChrome = Process.runSync(
             'dart',
             ['test', '-p', 'chrome', '--coverage=coverage'],
@@ -106,14 +110,15 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path
           print(resultChrome.stdout);
           print(resultChrome.stderr);
         }
-        print('Get test coverage for vm');
+        print('Run tests with coverage for vm');
         var resultVm = Process.runSync(
           'dart',
           ['test', '--coverage=coverage'],
           workingDirectory: package.directory.path,
         );
-        print(resultVm.stdout);
-        print(resultVm.stderr);
+        print('dart test stdout: ${resultVm.stdout}');
+        print('dart test stderr: ${resultVm.stderr}');
+        print('Compute coverage from runs');
         var resultLcov = Process.runSync(
           'dart',
           [
@@ -131,8 +136,8 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path
           ],
           workingDirectory: package.directory.path,
         );
-        print(resultLcov.stdout);
-        print(resultLcov.stderr);
+        print('dart coverage stdout: ${resultLcov.stdout}');
+        print('dart coverage stderr: ${resultLcov.stderr}');
         return parseLCOV(
           path.join(package.directory.path, 'coverage/lcov.info'),
           relativeTo: package.repository.baseDirectory.path,