Health workflow updates (#134)

* Group unlicensed files into touched/not touched in PR

* Add web test arg

* Look only at packages with changelogs

* Do not display unchanged license markdown conditionally

* Test if `test_web` works - to be reverted

* Revert "Test if `test_web` works - to be reverted"

This reverts commit aa9fcf9.

* Add changelog

* Changes as per review

* Change arg name

* Propagate arg name change

.github/workflows/health.yaml

@@ -12,6 +12,8 @@ name: Health
 # jobs:
 #   health:
 #     uses: dart-lang/ecosystem/.github/workflows/health.yaml@main
+#     with:
+#       coverage_web: true #If the coverage should run browser tests
 
 # Callers may optionally specify the version of the SDK to use when running the
 # health check. This can be useful if your package has a very recent minimum SDK
@@ -50,6 +52,16 @@ on:
         default: false
         type: boolean
         required: false
+      upload_coverage:
+        description: Whether to upload the coverage to coveralls
+        default: true
+        type: boolean
+        required: false
+      coverage_web:
+        description: Whether to run `dart test -p chrome` for coverage
+        default: false
+        type: boolean
+        required: false
 
 jobs:
   health:
@@ -90,9 +102,10 @@ jobs:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           ISSUE_NUMBER: ${{ github.event.number }}
           PR_LABELS: "${{ join(github.event.pull_request.labels.*.name) }}"
-        run: cd current_repo/ && dart pub global run firehose:health --checks ${{ inputs.checks }}
+        run: cd current_repo/ && dart pub global run firehose:health --checks ${{ inputs.checks }} ${{ fromJSON('{"true":"--test_web","false":""}')[inputs.test_web] }}
 
       - name: Upload coverage to Coveralls
+        if: ${{ inputs.upload_coverage }}
         uses: coverallsapp/github-action@c7885c00cb7ec0b8f9f5ff3f53cddb980f7a4412
         with:
           format: lcov

.github/workflows/health_internal.yaml

@@ -10,3 +10,4 @@ jobs:
     uses: ./.github/workflows/health.yaml
     with:
       local_debug: true
+      coverage_web: false

pkgs/firehose/CHANGELOG.md

@@ -1,5 +1,5 @@
-## 0.3.23-wip
-
+## 0.3.23
+- Tweak PR health workflow.
 - Shorten some text in the markdown summary table.
 
 ## 0.3.22

pkgs/firehose/bin/health.dart

@@ -13,9 +13,14 @@ void main(List<String> arguments) async {
       'checks',
       allowed: ['version', 'license', 'changelog', 'coverage'],
       help: 'Check PR health.',
+    )
+    ..addFlag(
+      'coverage_web',
+      help: 'Whether to run web tests for coverage',
     );
   var parsedArgs = argParser.parse(arguments);
   var checks = parsedArgs['checks'] as List<String>;
+  var coverageWeb = parsedArgs['coverage_web'] as bool;
 
-  await Health(Directory.current).healthCheck(checks);
+  await Health(Directory.current).healthCheck(checks, coverageWeb);
 }

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

@@ -8,20 +8,17 @@ import 'package:path/path.dart' as path;
 
 import '../github.dart';
 import '../repo.dart';
+import '../utils.dart';
 
 Future<Map<Package, List<GitFile>>> packagesWithoutChangelog(
     Github github) async {
   final repo = Repository();
   final packages = repo.locatePackages();
 
   final files = await github.listFilesForPR();
-  print('Collecting packages without changed changelogs:');
-  final packagesWithoutChangedChangelog = packages.where((package) {
-    var changelogPath = package.changelog.file.path;
-    var changelog = path.relative(changelogPath, from: Directory.current.path);
-    return !files.map((e) => e.relativePath).contains(changelog);
-  }).toList();
-  print('Done, found ${packagesWithoutChangedChangelog.length} packages.');
+
+  var packagesWithoutChangedChangelog =
+      collectPackagesWithoutChangelogChanges(packages, files);
 
   print('Collecting files without license headers in those packages:');
   var packagesWithChanges = <Package, List<GitFile>>{};
@@ -42,6 +39,19 @@ Done, found ${packagesWithChanges.length} packages with a need for a changelog.'
   return packagesWithChanges;
 }
 
+List<Package> collectPackagesWithoutChangelogChanges(
+    List<Package> packages, List<GitFile> files) {
+  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();
+  print('Done, found ${packagesWithoutChangedChangelog.length} packages.');
+  return packagesWithoutChangedChangelog;
+}
+
 bool fileNeedsEntryInChangelog(Package package, String file) {
   final directoryPath = package.directory.path;
   final directory = path.relative(directoryPath, from: Directory.current.path);

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

@@ -13,6 +13,10 @@ import '../utils.dart';
 import 'lcov.dart';
 
 class Coverage {
+  final bool coverageWeb;
+
+  Coverage(this.coverageWeb);
+
   Future<CoverageResult> compareCoverages() async {
     var files = await Github().listFilesForPR();
     var basePath = '../base_repo/';
@@ -88,14 +92,16 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path
           ['pub', 'get'],
           workingDirectory: package.directory.path,
         );
-        print('Get test coverage for web');
-        var resultChrome = Process.runSync(
-          'dart',
-          ['test', '-p', 'chrome', '--coverage=coverage'],
-          workingDirectory: package.directory.path,
-        );
-        print(resultChrome.stdout);
-        print(resultChrome.stderr);
+        if (coverageWeb) {
+          print('Get test coverage for web');
+          var resultChrome = Process.runSync(
+            'dart',
+            ['test', '-p', 'chrome', '--coverage=coverage'],
+            workingDirectory: package.directory.path,
+          );
+          print(resultChrome.stdout);
+          print(resultChrome.stderr);
+        }
         print('Get test coverage for vm');
         var resultVm = Process.runSync(
           'dart',

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

@@ -7,6 +7,7 @@
 import 'dart:io';
 import 'dart:math';
 
+import 'package:collection/collection.dart';
 import 'package:firehose/firehose.dart';
 
 import '../github.dart';
@@ -34,7 +35,7 @@ class Health {
 
   Health(this.directory);
 
-  Future<void> healthCheck(List args) async {
+  Future<void> healthCheck(List args, bool coverageweb) async {
     var github = Github();
 
     // Do basic validation of our expected env var.
@@ -55,13 +56,13 @@ class Health {
         validateCheck,
       if (args.contains('license') &&
           !github.prLabels.contains('skip-license-check'))
-        (Github _) => licenseCheck(),
+        licenseCheck,
       if (args.contains('changelog') &&
           !github.prLabels.contains('skip-changelog-check'))
         changelogCheck,
       if (args.contains('coverage') &&
           !github.prLabels.contains('skip-coverage-check'))
-        coverageCheck,
+        (Github github) => coverageCheck(github, coverageweb),
     ];
 
     var checked =
@@ -90,25 +91,46 @@ Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automati
     );
   }
 
-  Future<HealthCheckResult> licenseCheck() async {
-    var filePaths = await getFilesWithoutLicenses(Directory.current);
+  Future<HealthCheckResult> licenseCheck(Github github) async {
+    var files = await Github().listFilesForPR();
+    var allFilePaths = await getFilesWithoutLicenses(Directory.current);
 
+    var groupedPaths = allFilePaths
+        .groupListsBy((path) => files.any((f) => f.relativePath == path));
+
+    var unchangedFilesPaths = groupedPaths[false] ?? [];
+    var unchangedMarkdown = '''
+<details>
+<summary>
+Unrelated files missing license headers
+</summary>
+
+| Files |
+| :--- |
+${unchangedFilesPaths.map((e) => '|$e|').join('\n')}
+</details>
+''';
+
+    var changedFilesPaths = groupedPaths[true] ?? [];
     var markdownResult = '''
 ```
 $license
 ```
 
 | Files |
 | :--- |
-${filePaths.isNotEmpty ? filePaths.map((e) => '|$e|').join('\n') : '| _no missing headers_  |'}
+${changedFilesPaths.isNotEmpty ? changedFilesPaths.map((e) => '|$e|').join('\n') : '| _no missing headers_  |'}
 
 All source files should start with a [license header](https://github.com/dart-lang/ecosystem/wiki/License-Header).
+
+${unchangedFilesPaths.isNotEmpty ? unchangedMarkdown : ''}
+
 ''';
 
     return HealthCheckResult(
       'license',
       _licenseBotTag,
-      filePaths.isNotEmpty ? Severity.error : Severity.success,
+      changedFilesPaths.isNotEmpty ? Severity.error : Severity.success,
       markdownResult,
     );
   }
@@ -132,8 +154,11 @@ Changes to files need to be [accounted for](https://github.com/dart-lang/ecosyst
     );
   }
 
-  Future<HealthCheckResult> coverageCheck(Github github) async {
-    var coverage = await Coverage().compareCoverages();
+  Future<HealthCheckResult> coverageCheck(
+    Github github,
+    bool coverageWeb,
+  ) async {
+    var coverage = await Coverage(coverageWeb).compareCoverages();
 
     var markdownResult = '''
 | File | Coverage |

pkgs/firehose/lib/src/utils.dart

@@ -5,6 +5,8 @@
 import 'dart:convert';
 import 'dart:io';
 
+import 'package:path/path.dart' as path;
+
 /// Execute the given CLI command asynchronously, streaming stdout and stderr to
 /// the console.
 ///
@@ -109,6 +111,11 @@ bool expectEnv(String? value, String name) {
   }
 }
 
+extension FileExtension on File {
+  String get relativePath =>
+      path.relative(this.path, from: Directory.current.path);
+}
+
 enum Severity {
   success(':heavy_check_mark:'),
   info(':heavy_check_mark:'),

pkgs/firehose/pubspec.yaml

@@ -1,6 +1,6 @@
 name: firehose
 description: A tool to automate publishing of Pub packages from GitHub actions.
-version: 0.3.23-wip
+version: 0.3.23
 repository: https://github.com/dart-lang/ecosystem/tree/main/pkgs/firehose
 
 environment:
@@ -12,6 +12,7 @@ executables:
 
 dependencies:
   args: ^2.3.0
+  collection: ^1.17.2
   http: ^0.13.0
   path: ^1.8.0
   pub_semver: ^2.1.0

pkgs/firehose/test/coverage_test.dart

@@ -44,6 +44,8 @@ void main() {
 }
 
 class FakeHealth extends Coverage {
+  FakeHealth() : super(true);
+
   @override
   Map<String, double> getCoverage(Package? package) {
     Map<String, double> result;