diff --git a/.github/workflows/health.yaml b/.github/workflows/health.yaml index ae3f2be1..b0aa1bba 100644 --- a/.github/workflows/health.yaml +++ b/.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 diff --git a/.github/workflows/health_internal.yaml b/.github/workflows/health_internal.yaml index f32f8380..036e67a0 100644 --- a/.github/workflows/health_internal.yaml +++ b/.github/workflows/health_internal.yaml @@ -10,3 +10,4 @@ jobs: 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 fc42f139..5ef54877 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 a4d021e9..25b80524 100644 --- a/pkgs/firehose/bin/health.dart +++ b/pkgs/firehose/bin/health.dart @@ -13,9 +13,14 @@ void main(List 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; + 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 d13872b1..10534a03 100644 --- a/pkgs/firehose/lib/src/health/changelog.dart +++ b/pkgs/firehose/lib/src/health/changelog.dart @@ -8,6 +8,7 @@ import 'package:path/path.dart' as path; import '../github.dart'; import '../repo.dart'; +import '../utils.dart'; Future>> packagesWithoutChangelog( Github github) async { @@ -15,13 +16,9 @@ Future>> packagesWithoutChangelog( 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 = >{}; @@ -42,6 +39,19 @@ Done, found ${packagesWithChanges.length} packages with a need for a changelog.' return packagesWithChanges; } +List collectPackagesWithoutChangelogChanges( + List packages, List 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 e82aa3e1..018be80e 100644 --- a/pkgs/firehose/lib/src/health/coverage.dart +++ b/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 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', diff --git a/pkgs/firehose/lib/src/health/health.dart b/pkgs/firehose/lib/src/health/health.dart index c1a399e6..973a6d81 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 @@ class Health { Health(this.directory); - Future healthCheck(List args) async { + Future 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,9 +91,27 @@ Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automati ); } - Future licenseCheck() async { - var filePaths = await getFilesWithoutLicenses(Directory.current); + Future 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 = ''' +
+ +Unrelated files missing license headers + + +| Files | +| :--- | +${unchangedFilesPaths.map((e) => '|$e|').join('\n')} +
+'''; + + var changedFilesPaths = groupedPaths[true] ?? []; var markdownResult = ''' ``` $license @@ -100,15 +119,18 @@ $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 coverageCheck(Github github) async { - var coverage = await Coverage().compareCoverages(); + Future 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 1cd5e80b..cc68c3c1 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 @@ 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:'), diff --git a/pkgs/firehose/pubspec.yaml b/pkgs/firehose/pubspec.yaml index 36617a59..3f061184 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 @@ executables: 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 1641c8e4..b384f06c 100644 --- a/pkgs/firehose/test/coverage_test.dart +++ b/pkgs/firehose/test/coverage_test.dart @@ -44,6 +44,8 @@ void main() { } class FakeHealth extends Coverage { + FakeHealth() : super(true); + @override Map getCoverage(Package? package) { Map result;