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 aa9fcf9f1ce778ee15782b9ccfc12fd9d6efa21c.
* Add changelog
* Changes as per review
* Change arg name
* Propagate arg name change
diff --git a/.github/workflows/health.yaml b/.github/workflows/health.yaml
index ae3f2be..b0aa1bb 100644
--- a/.github/workflows/health.yaml
+++ b/.github/workflows/health.yaml
@@ -12,6 +12,8 @@
# 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 @@
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 @@
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
diff --git a/.github/workflows/health_internal.yaml b/.github/workflows/health_internal.yaml
index f32f838..036e67a 100644
--- a/.github/workflows/health_internal.yaml
+++ b/.github/workflows/health_internal.yaml
@@ -10,3 +10,4 @@
uses: ./.github/workflows/health.yaml
with:
local_debug: true
+ coverage_web: false
diff --git a/pkgs/firehose/CHANGELOG.md b/pkgs/firehose/CHANGELOG.md
index fc42f13..5ef5487 100644
--- a/pkgs/firehose/CHANGELOG.md
+++ b/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
diff --git a/pkgs/firehose/bin/health.dart b/pkgs/firehose/bin/health.dart
index a4d021e..25b8052 100644
--- a/pkgs/firehose/bin/health.dart
+++ b/pkgs/firehose/bin/health.dart
@@ -13,9 +13,14 @@
'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);
}
diff --git a/pkgs/firehose/lib/src/health/changelog.dart b/pkgs/firehose/lib/src/health/changelog.dart
index d13872b..10534a0 100644
--- a/pkgs/firehose/lib/src/health/changelog.dart
+++ b/pkgs/firehose/lib/src/health/changelog.dart
@@ -8,6 +8,7 @@
import '../github.dart';
import '../repo.dart';
+import '../utils.dart';
Future<Map<Package, List<GitFile>>> packagesWithoutChangelog(
Github github) async {
@@ -15,13 +16,9 @@
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 @@
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);
diff --git a/pkgs/firehose/lib/src/health/coverage.dart b/pkgs/firehose/lib/src/health/coverage.dart
index e82aa3e..018be80 100644
--- a/pkgs/firehose/lib/src/health/coverage.dart
+++ b/pkgs/firehose/lib/src/health/coverage.dart
@@ -13,6 +13,10 @@
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 @@
['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',
diff --git a/pkgs/firehose/lib/src/health/health.dart b/pkgs/firehose/lib/src/health/health.dart
index c1a399e..973a6d8 100644
--- a/pkgs/firehose/lib/src/health/health.dart
+++ b/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 @@
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 @@
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,9 +91,27 @@
);
}
- 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
@@ -100,15 +119,18 @@
| 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 @@
);
}
- 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 |
diff --git a/pkgs/firehose/lib/src/utils.dart b/pkgs/firehose/lib/src/utils.dart
index 1cd5e80..cc68c3c 100644
--- a/pkgs/firehose/lib/src/utils.dart
+++ b/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 @@
}
}
+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:'),
diff --git a/pkgs/firehose/pubspec.yaml b/pkgs/firehose/pubspec.yaml
index 36617a5..3f06118 100644
--- a/pkgs/firehose/pubspec.yaml
+++ b/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 @@
dependencies:
args: ^2.3.0
+ collection: ^1.17.2
http: ^0.13.0
path: ^1.8.0
pub_semver: ^2.1.0
diff --git a/pkgs/firehose/test/coverage_test.dart b/pkgs/firehose/test/coverage_test.dart
index 1641c8e..b384f06 100644
--- a/pkgs/firehose/test/coverage_test.dart
+++ b/pkgs/firehose/test/coverage_test.dart
@@ -44,6 +44,8 @@
}
class FakeHealth extends Coverage {
+ FakeHealth() : super(true);
+
@override
Map<String, double> getCoverage(Package? package) {
Map<String, double> result;
