From 8749a2bd0af7900a75d7c031559ed43cdddc4ffd Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 6 Dec 2024 13:43:10 +0100 Subject: [PATCH] Updates to `health.yaml` (#310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Run health workflow on all packages if it is changed - Specify Flutter packages in the repo, to only have a single workflow file - Compare to last published version in breaking check. This is a breaking change, as the `use-flutter` argument is not existing anymore. After landing this, all `dart-lang/` repos will have to be updated. As part of that update, we should maybe have the `health.yaml`s depend on a hash instead of the `main` branch. --- - [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/blob/main/docs/External-Package-Maintenance.md#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
--------- Co-authored-by: Devon Carew --- .github/workflows/health.yaml | 28 +-- .github/workflows/health_base.yaml | 37 ++-- pkgs/firehose/CHANGELOG.md | 3 + pkgs/firehose/bin/health.dart | 15 +- pkgs/firehose/lib/firehose.dart | 2 +- pkgs/firehose/lib/src/health/changelog.dart | 2 +- pkgs/firehose/lib/src/health/coverage.dart | 20 +-- pkgs/firehose/lib/src/health/health.dart | 170 +++++++++++++----- pkgs/firehose/lib/src/repo.dart | 13 +- pkgs/firehose/test/coverage_test.dart | 2 +- pkgs/firehose/test/health_test.dart | 113 ++++++++---- .../golden/comment_breaking_healthchanged.md | 16 ++ .../golden/comment_changelog_healthchanged.md | 16 ++ .../golden/comment_coverage_healthchanged.md | 23 +++ .../comment_do-not-submit_healthchanged.md | 15 ++ .../golden/comment_leaking_healthchanged.md | 15 ++ .../golden/comment_license_healthchanged.md | 34 ++++ 17 files changed, 401 insertions(+), 123 deletions(-) create mode 100644 pkgs/firehose/test_data/golden/comment_breaking_healthchanged.md create mode 100644 pkgs/firehose/test_data/golden/comment_changelog_healthchanged.md create mode 100644 pkgs/firehose/test_data/golden/comment_coverage_healthchanged.md create mode 100644 pkgs/firehose/test_data/golden/comment_do-not-submit_healthchanged.md create mode 100644 pkgs/firehose/test_data/golden/comment_leaking_healthchanged.md create mode 100644 pkgs/firehose/test_data/golden/comment_license_healthchanged.md diff --git a/.github/workflows/health.yaml b/.github/workflows/health.yaml index c0a4e8fb..b116f533 100644 --- a/.github/workflows/health.yaml +++ b/.github/workflows/health.yaml @@ -26,7 +26,7 @@ name: Health # warn_on: "license,coverage,breaking,leaking" # coverage_web: false # upload_coverage: false -# use-flutter: true +# flutter_packages: "pkgs/my_flutter_package" # ignore_license: "**.g.dart" # ignore_coverage: "**.mock.dart,**.g.dart" # ignore_packages: "pkgs/helper_package" @@ -42,6 +42,12 @@ on: # parameter is not required; it defaults to `stable` - using the most recent # stable release of the Dart SDK. sdk: + description: >- + The Dart SDK version, either a semver or one of `dev`, `stable` etc. + default: "stable" + required: false + type: string + channel: description: >- The channel, or a specific version from a channel, to install ('2.19.0','stable', 'beta', 'dev'). Using one of the three channels @@ -81,11 +87,11 @@ on: default: false type: boolean required: false - use-flutter: - description: Whether to setup Flutter in this workflow. - default: false + flutter_packages: + description: List of packages depending on Flutter. + default: "\"\"" required: false - type: boolean + type: string ignore_license: description: Which files to ignore for the license check. default: "\"\"" @@ -122,7 +128,7 @@ jobs: fail_on: ${{ inputs.fail_on }} warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} - use-flutter: ${{ inputs.use-flutter }} + flutter_packages: ${{ inputs.flutter_packages }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -135,7 +141,7 @@ jobs: fail_on: ${{ inputs.fail_on }} warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} - use-flutter: ${{ inputs.use-flutter }} + flutter_packages: ${{ inputs.flutter_packages }} ignore_license: ${{ inputs.ignore_license }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -151,7 +157,7 @@ jobs: upload_coverage: ${{ inputs.upload_coverage }} coverage_web: ${{ inputs.coverage_web }} local_debug: ${{ inputs.local_debug }} - use-flutter: ${{ inputs.use-flutter }} + flutter_packages: ${{ inputs.flutter_packages }} ignore_coverage: ${{ inputs.ignore_coverage }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -166,7 +172,7 @@ jobs: fail_on: ${{ inputs.fail_on }} warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} - use-flutter: ${{ inputs.use-flutter }} + flutter_packages: ${{ inputs.flutter_packages }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -179,7 +185,7 @@ jobs: fail_on: ${{ inputs.fail_on }} warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} - use-flutter: ${{ inputs.use-flutter }} + flutter_packages: ${{ inputs.flutter_packages }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -192,7 +198,7 @@ jobs: fail_on: ${{ inputs.fail_on }} warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} - use-flutter: ${{ inputs.use-flutter }} + flutter_packages: ${{ inputs.flutter_packages }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} diff --git a/.github/workflows/health_base.yaml b/.github/workflows/health_base.yaml index c2fae51e..6b7c4c16 100644 --- a/.github/workflows/health_base.yaml +++ b/.github/workflows/health_base.yaml @@ -15,6 +15,14 @@ on: default: "stable" required: false type: string + channel: + description: >- + The channel, or a specific version from a channel, to install + ('2.19.0','stable', 'beta', 'dev'). Using one of the three named channels + will give you the latest version published to that channel. + default: "stable" + required: false + type: string check: description: What to check for in the PR health check - any of "changelog,license,coverage,breaking,do-not-submit,leaking" type: string @@ -44,11 +52,11 @@ on: default: false type: boolean required: false - use-flutter: - description: Whether to setup Flutter in this workflow. - default: false + flutter_packages: + description: List of packages depending on Flutter. + default: "\"\"" required: false - type: boolean + type: string ignore_license: description: Which files to ignore for the license check. default: "\"\"" @@ -98,23 +106,29 @@ jobs: if: ${{ inputs.check == 'coverage' }} || ${{ inputs.check == 'breaking' }} - run: mkdir -p current_repo/output/ - - - uses: subosito/flutter-action@74af56c5ed2697ba4621264652728e8d217e53d3 - if: ${{ inputs.use-flutter }} - with: - channel: ${{ inputs.sdk }} - uses: dart-lang/setup-dart@e630b99d28a3b71860378cafdc2a067c71107f94 - if: ${{ !inputs.use-flutter }} with: sdk: ${{ inputs.sdk }} + - uses: subosito/flutter-action@74af56c5ed2697ba4621264652728e8d217e53d3 + if: ${{ inputs.flutter_packages != '' }} + with: + channel: ${{ inputs.channel }} + + - name: Check Dart installs whereis + run: whereis dart + + - name: Check Dart installs which + run: which dart + - name: Install coverage run: dart pub global activate coverage if: ${{ inputs.check == 'coverage' }} - name: Install firehose - run: dart pub global activate firehose + run: dart pub global activate --source git https://github.com/dart-lang/ecosystem --git-path pkgs/firehose/ --git-ref=forceFlutter + # DO-NOT-SUBMIT if: ${{ !inputs.local_debug }} - name: Install local firehose @@ -139,6 +153,7 @@ jobs: ${{ fromJSON('{"true":"--coverage_web","false":""}')[inputs.coverage_web] }} \ --fail_on ${{ inputs.fail_on }} \ --warn_on ${{ inputs.warn_on }} \ + --flutter_packages ${{ inputs.flutter_packages }} \ --ignore_license ${{ inputs.ignore_license }} \ --ignore_coverage ${{ inputs.ignore_coverage }} \ --ignore_packages ${{ inputs.ignore_packages }} \ diff --git a/pkgs/firehose/CHANGELOG.md b/pkgs/firehose/CHANGELOG.md index 632fb5e9..bf8a747b 100644 --- a/pkgs/firehose/CHANGELOG.md +++ b/pkgs/firehose/CHANGELOG.md @@ -3,6 +3,9 @@ - Remove the `version` pubspec checks (these largely duplicate the feedback provided by publishing automation). - Set minimum SDK version to `3.5.0` because of the `dart_apitool` dependency. +- Run health workflow on all packages if it is changed. +- Specify Flutter packages in the repo, to only have a single workflow file. +- Compare to last published version in breaking check. ## 0.9.3 diff --git a/pkgs/firehose/bin/health.dart b/pkgs/firehose/bin/health.dart index 697dd74c..122e0444 100644 --- a/pkgs/firehose/bin/health.dart +++ b/pkgs/firehose/bin/health.dart @@ -48,17 +48,23 @@ void main(List arguments) async { ..addFlag( 'coverage_web', help: 'Whether to run web tests for coverage', + ) + ..addMultiOption( + 'flutter_packages', + defaultsTo: [], + help: 'The Flutter packages in this repo', ); final parsedArgs = argParser.parse(arguments); - final checkStr = parsedArgs['check'] as String; + final checkStr = parsedArgs.option('check'); final check = Check.values.firstWhere((c) => c.name == checkStr); - final warnOn = parsedArgs['warn_on'] as List; - final failOn = parsedArgs['fail_on'] as List; + final warnOn = parsedArgs.multiOption('warn_on'); + final failOn = parsedArgs.multiOption('fail_on'); + final flutterPackages = _listNonEmpty(parsedArgs, 'flutter_packages'); final ignorePackages = _listNonEmpty(parsedArgs, 'ignore_packages'); final ignoreLicense = _listNonEmpty(parsedArgs, 'ignore_license'); final ignoreCoverage = _listNonEmpty(parsedArgs, 'ignore_coverage'); final experiments = _listNonEmpty(parsedArgs, 'experiments'); - final coverageWeb = parsedArgs['coverage_web'] as bool; + final coverageWeb = parsedArgs.flag('coverage_web'); if (warnOn.toSet().intersection(failOn.toSet()).isNotEmpty) { throw ArgumentError('The checks for which warnings are displayed and the ' 'checks which lead to failure must be disjoint.'); @@ -74,6 +80,7 @@ void main(List arguments) async { ignoreCoverage, experiments, GithubApi(), + flutterPackages, ).healthCheck(); } diff --git a/pkgs/firehose/lib/firehose.dart b/pkgs/firehose/lib/firehose.dart index 618dbb6f..f7d7944b 100644 --- a/pkgs/firehose/lib/firehose.dart +++ b/pkgs/firehose/lib/firehose.dart @@ -100,7 +100,7 @@ Saving existing comment id $existingCommentId to file ${idFile.path}'''); Future verify(GithubApi github) async { var repo = Repository(directory); - var packages = repo.locatePackages(ignoredPackages); + var packages = repo.locatePackages(ignore: ignoredPackages); var pub = Pub(); diff --git a/pkgs/firehose/lib/src/health/changelog.dart b/pkgs/firehose/lib/src/health/changelog.dart index 95ad14ab..e725ff5a 100644 --- a/pkgs/firehose/lib/src/health/changelog.dart +++ b/pkgs/firehose/lib/src/health/changelog.dart @@ -16,7 +16,7 @@ Future>> packagesWithoutChangelog( Directory directory, ) async { final repo = Repository(directory); - final packages = repo.locatePackages(ignoredPackages); + final packages = repo.locatePackages(ignore: ignoredPackages); final files = await github.listFilesForPR(directory, ignoredPackages); diff --git a/pkgs/firehose/lib/src/health/coverage.dart b/pkgs/firehose/lib/src/health/coverage.dart index bb0610dc..c4d76781 100644 --- a/pkgs/firehose/lib/src/health/coverage.dart +++ b/pkgs/firehose/lib/src/health/coverage.dart @@ -20,6 +20,7 @@ class Coverage { final List ignoredPackages; final Directory directory; final List experiments; + final String dartExecutable; Coverage( this.coverageWeb, @@ -27,17 +28,12 @@ class Coverage { this.ignoredPackages, this.directory, this.experiments, + this.dartExecutable, ); - Future compareCoverages( - GithubApi github, Directory base) async { - var files = await github.listFilesForPR(directory, ignoredFiles); - return compareCoveragesFor(files, base); - } - CoverageResult compareCoveragesFor(List files, Directory base) { var repository = Repository(directory); - var packages = repository.locatePackages(ignoredPackages); + var packages = repository.locatePackages(ignore: ignoredPackages); print('Found packages $packages at $directory'); var filesOfInterest = files @@ -49,7 +45,7 @@ class Coverage { print('The files of interest are $filesOfInterest'); var baseRepository = Repository(base); - var basePackages = baseRepository.locatePackages(ignoredFiles); + var basePackages = baseRepository.locatePackages(ignore: ignoredFiles); print('Found packages $basePackages at $base'); var changedPackages = packages @@ -105,7 +101,7 @@ class Coverage { print(''' Get coverage for ${package.name} by running coverage in ${package.directory.path}'''); Process.runSync( - 'dart', + dartExecutable, [ if (experiments.isNotEmpty) '--enable-experiment=${experiments.join(',')}', @@ -117,7 +113,7 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path if (coverageWeb) { print('Run tests with coverage for web'); var resultChrome = Process.runSync( - 'dart', + dartExecutable, [ if (experiments.isNotEmpty) '--enable-experiment=${experiments.join(',')}', @@ -136,7 +132,7 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path print('Run tests with coverage for vm'); var resultVm = Process.runSync( - 'dart', + dartExecutable, [ if (experiments.isNotEmpty) '--enable-experiment=${experiments.join(',')}', @@ -152,7 +148,7 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path print('Compute coverage from runs'); var resultLcov = Process.runSync( - 'dart', + dartExecutable, [ 'pub', 'global', diff --git a/pkgs/firehose/lib/src/health/health.dart b/pkgs/firehose/lib/src/health/health.dart index 8850519d..18dd388f 100644 --- a/pkgs/firehose/lib/src/health/health.dart +++ b/pkgs/firehose/lib/src/health/health.dart @@ -47,26 +47,40 @@ class Health { List ignoredLicense, List ignoredCoverage, this.experiments, - this.github, { + this.github, + List flutterPackages, { Directory? base, String? comment, this.log = printLogger, - }) : ignoredPackages = ignoredPackages - .map((pattern) => Glob(pattern, recursive: true)) - .toList(), - ignoredFilesForCoverage = ignoredCoverage - .map((pattern) => Glob(pattern, recursive: true)) - .toList(), - ignoredFilesForLicense = ignoredLicense - .map((pattern) => Glob(pattern, recursive: true)) - .toList(), + }) : ignoredPackages = toGlobs(ignoredPackages), + flutterPackageGlobs = toGlobs(flutterPackages), + ignoredFilesForCoverage = toGlobs(ignoredCoverage), + ignoredFilesForLicense = toGlobs(ignoredLicense), baseDirectory = base ?? Directory('../base_repo'), commentPath = comment ?? path.join( directory.path, 'output', 'comment.md', - ); + ) { + flutterExecutable = + (Process.runSync('which', ['-a', 'flutter']).stdout as String) + .split('\n') + .where((element) => element.isNotEmpty) + .firstOrNull; + + var dartExecutables = + (Process.runSync('which', ['-a', 'dart']).stdout as String) + .split('\n') + .where((element) => element.isNotEmpty); + dartExecutable = dartExecutables + .sortedBy((path) => path.contains('flutter').toString()) + .first; + } + + static List toGlobs(List ignoredPackages) => + ignoredPackages.map((pattern) => Glob(pattern, recursive: true)).toList(); + final GithubApi github; final Check check; @@ -76,10 +90,17 @@ class Health { final List ignoredPackages; final List ignoredFilesForLicense; final List ignoredFilesForCoverage; + final List flutterPackageGlobs; final Directory baseDirectory; final List experiments; final Logger log; + late final String dartExecutable; + late final String? flutterExecutable; + + String executable(bool isFlutter) => + isFlutter ? flutterExecutable ?? dartExecutable : dartExecutable; + Future healthCheck() async { // Do basic validation of our expected env var. if (!expectEnv(github.repoSlug?.fullName, 'GITHUB_REPOSITORY')) return; @@ -91,6 +112,7 @@ class Health { log(' warnOn: $warnOn'); log(' failOn: $failOn'); log(' coverageweb: $coverageweb'); + log(' flutterPackages: $flutterPackageGlobs'); log(' ignoredPackages: $ignoredPackages'); log(' ignoredForLicense: $ignoredFilesForLicense'); log(' ignoredForCoverage: $ignoredFilesForCoverage'); @@ -127,33 +149,32 @@ class Health { }; Future breakingCheck() async { - final filesInPR = await github.listFilesForPR(directory, ignoredPackages); + final filesInPR = await listFilesInPRorAll(ignoredPackages); final changeForPackage = {}; - for (var package in packagesContaining(filesInPR, ignoredPackages)) { - log('Look for changes in $package with base $baseDirectory'); + final flutterPackages = + packagesContaining(filesInPR, only: flutterPackageGlobs); + + for (var package + in packagesContaining(filesInPR, ignore: ignoredPackages)) { + log('Look for changes in $package'); var relativePath = path.relative(package.directory.path, from: directory.path); - var baseRelativePath = path.relative( - path.join(baseDirectory.path, relativePath), - from: directory.path); var tempDirectory = Directory.systemTemp.createTempSync(); var reportPath = path.join(tempDirectory.path, 'report.json'); - var runApiTool = Process.runSync( - 'dart', + runDashProcess( + flutterPackages, + package, [ ...['pub', 'global', 'run'], 'dart_apitool:main', 'diff', '--no-check-sdk-version', - ...['--old', baseRelativePath], + ...['--old', getCurrentVersionOfPackage(package)], ...['--new', relativePath], ...['--report-format', 'json'], ...['--report-file-path', reportPath], ], - workingDirectory: directory.path, ); - log(runApiTool.stderr as String); - log(runApiTool.stdout as String); var fullReportString = File(reportPath).readAsStringSync(); var decoded = jsonDecode(fullReportString) as Map; @@ -185,6 +206,22 @@ ${changeForPackage.entries.map((e) => '|${e.key.name}|${e.value.toMarkdownRow()} ); } + String getCurrentVersionOfPackage(Package package) => 'pub://${package.name}'; + + ProcessResult runDashProcess( + List flutterPackages, Package package, List arguments) { + var exec = executable(flutterPackages.contains(package)); + log('Running `$exec ${arguments.join(' ')}` in ${directory.path}'); + var runApiTool = Process.runSync( + exec, + arguments, + workingDirectory: directory.path, + ); + log(runApiTool.stderr as String); + log(runApiTool.stdout as String); + return runApiTool; + } + BreakingLevel _breakingLevel(Map report) { BreakingLevel breakingLevel; if ((report['noChangesDetected'] as bool?) ?? false) { @@ -200,36 +237,49 @@ ${changeForPackage.entries.map((e) => '|${e.key.name}|${e.value.toMarkdownRow()} } Future leakingCheck() async { - final filesInPR = await github.listFilesForPR(directory, ignoredPackages); + var filesInPR = await listFilesInPRorAll(ignoredPackages); final leaksForPackage = >{}; - for (var package in packagesContaining(filesInPR, ignoredPackages)) { + + final flutterPackages = + packagesContaining(filesInPR, only: flutterPackageGlobs); + for (var package in packagesContaining(filesInPR)) { log('Look for leaks in $package'); var relativePath = path.relative(package.directory.path, from: directory.path); var tempDirectory = Directory.systemTemp.createTempSync(); var reportPath = path.join(tempDirectory.path, 'leaks.json'); - var runApiTool = Process.runSync( - 'dart', + var runApiTool = runDashProcess( + flutterPackages, + package, [ ...['pub', 'global', 'run'], 'dart_apitool:main', 'extract', ...['--input', relativePath], ...['--output', reportPath], - '--set-exit-on-missing-export', ], - workingDirectory: directory.path, ); - log(runApiTool.stderr as String); - log(runApiTool.stdout as String); - var fullReportString = File(reportPath).readAsStringSync(); - var decoded = jsonDecode(fullReportString) as Map; - var leaks = decoded['missingEntryPoints'] as List; + if (runApiTool.exitCode == 0) { + var fullReportString = await File(reportPath).readAsString(); + var decoded = jsonDecode(fullReportString) as Map; + var leaks = decoded['missingEntryPoints'] as List; - log('Leaking symbols in API:\n$leaks'); - if (leaks.isNotEmpty) { - leaksForPackage[package] = leaks.cast(); + log('Leaking symbols in API:\n$leaks'); + if (leaks.isNotEmpty) { + leaksForPackage[package] = leaks.cast(); + } + } else { + throw ProcessException( + 'Api tool finished with exit code ${runApiTool.exitCode}', + [ + ...['pub', 'global', 'run'], + 'dart_apitool:main', + 'extract', + ...['--input', relativePath], + ...['--output', reportPath], + ], + ); } } return HealthCheckResult( @@ -248,7 +298,7 @@ ${leaksForPackage.entries.map((e) => '|${e.key.name}|${e.value.join('
')}|'). } Future licenseCheck() async { - var files = await github.listFilesForPR(directory, ignoredPackages); + var files = await listFilesInPRorAll(ignoredPackages); var allFilePaths = await getFilesWithoutLicenses( directory, [...ignoredFilesForLicense, ...ignoredPackages], @@ -294,6 +344,13 @@ ${unchangedFilesPaths.isNotEmpty ? unchangedMarkdown : ''} ); } + bool healthYamlChanged(List files) => files + .where((file) => + [FileStatus.added, FileStatus.modified].contains(file.status)) + .any((file) => + file.filename.endsWith('health.yaml') || + file.filename.endsWith('health.yml')); + Future changelogCheck() async { var filePaths = await packagesWithoutChangelog( github, @@ -322,7 +379,7 @@ Changes to files need to be [accounted for](https://github.com/dart-lang/ecosyst const supportedExtensions = ['.dart', '.json', '.md', '.txt']; final body = await github.pullrequestBody(); - final files = await github.listFilesForPR(directory, ignoredPackages); + var files = await listFilesInPRorAll(ignoredPackages); log('Checking for DO_NOT${'_'}SUBMIT strings: $files'); final filesWithDNS = files .where((file) => @@ -353,26 +410,44 @@ ${filesWithDNS.map((e) => e.filename).map((e) => '|$e|').join('\n')} ); } + Future> listFilesInPRorAll(List ignore) async { + final files = await github.listFilesForPR(directory, ignore); + return healthYamlChanged(files) ? await _getAllFiles(ignore) : files; + } + + Future> _getAllFiles(List ignored) async => + await directory + .list(recursive: true) + .where((entity) => entity is File) + .map((file) => path.relative(file.path, from: directory.path)) + .where((file) => ignored.none((glob) => glob.matches(file))) + .map((file) => GitFile(file, FileStatus.added, directory)) + .toList(); + Future coverageCheck() async { - var coverage = await Coverage( + var coverage = Coverage( coverageweb, ignoredFilesForCoverage, ignoredPackages, directory, experiments, - ).compareCoverages(github, directory); + dartExecutable, + ); + + var files = await listFilesInPRorAll(ignoredPackages); + var coverageResult = coverage.compareCoveragesFor(files, baseDirectory); var markdownResult = ''' | File | Coverage | | :--- | :--- | -${coverage.coveragePerFile.entries.map((e) => '|${e.key}| ${e.value.toMarkdown()} |').join('\n')} +${coverageResult.coveragePerFile.entries.map((e) => '|${e.key}| ${e.value.toMarkdown()} |').join('\n')} This check for [test coverage](https://github.com/dart-lang/ecosystem/wiki/Test-Coverage) is informational (issues shown here will not fail the PR). '''; return HealthCheckResult( Check.coverage, - Severity.values[coverage.coveragePerFile.values + Severity.values[coverageResult.coveragePerFile.values .map((change) => change.severity.index) .fold(0, max)], markdownResult, @@ -415,12 +490,13 @@ ${isWorseThanInfo ? 'This check can be disabled by tagging the PR with `skip-${r } List packagesContaining( - List filesInPR, - List ignoredPackages, - ) { + List filesInPR, { + List? ignore, + List? only, + }) { var files = filesInPR.where((element) => element.status.isRelevant); return Repository(directory) - .locatePackages(ignoredPackages) + .locatePackages(ignore: ignore, only: only) .where((package) => files.any((file) => path.isWithin(package.directory.path, file.pathInRepository))) .toList(); diff --git a/pkgs/firehose/lib/src/repo.dart b/pkgs/firehose/lib/src/repo.dart index 1d5fea11..3e174067 100644 --- a/pkgs/firehose/lib/src/repo.dart +++ b/pkgs/firehose/lib/src/repo.dart @@ -4,6 +4,7 @@ import 'dart:io'; +import 'package:collection/collection.dart'; import 'package:glob/glob.dart'; import 'package:path/path.dart' as path; import 'package:pub_semver/pub_semver.dart'; @@ -40,11 +41,17 @@ class Repository { /// `publish_to: none` key. /// /// Once we find a package, we don't look for packages in sub-directories. - List locatePackages([List ignore = const []]) { + List locatePackages({List? only, List? ignore}) { final packages = []; _recurseAndGather(baseDirectory, packages); - packages.removeWhere((package) => ignore.any((glob) => glob.matches( - path.relative(package.directory.path, from: baseDirectory.path)))); + if (ignore != null) { + packages.removeWhere((package) => ignore.any((glob) => glob.matches( + path.relative(package.directory.path, from: baseDirectory.path)))); + } + if (only != null) { + packages.removeWhere((package) => only.none((glob) => glob.matches( + path.relative(package.directory.path, from: baseDirectory.path)))); + } packages.sort((a, b) => a.name.compareTo(b.name)); return packages; } diff --git a/pkgs/firehose/test/coverage_test.dart b/pkgs/firehose/test/coverage_test.dart index 9a066635..63abdbcf 100644 --- a/pkgs/firehose/test/coverage_test.dart +++ b/pkgs/firehose/test/coverage_test.dart @@ -43,7 +43,7 @@ void main() { } class FakeHealth extends Coverage { - FakeHealth() : super(true, [], [], Directory.current, []); + FakeHealth() : super(true, [], [], Directory.current, [], 'dart'); @override Map getCoverage(Package? package) { diff --git a/pkgs/firehose/test/health_test.dart b/pkgs/firehose/test/health_test.dart index 455331bd..d17e6db2 100644 --- a/pkgs/firehose/test/health_test.dart +++ b/pkgs/firehose/test/health_test.dart @@ -7,6 +7,7 @@ import 'dart:io'; import 'package:collection/collection.dart'; import 'package:firehose/src/github.dart'; import 'package:firehose/src/health/health.dart'; +import 'package:firehose/src/repo.dart'; import 'package:github/src/common/model/repos.dart'; import 'package:glob/glob.dart'; import 'package:path/path.dart' as p; @@ -14,37 +15,39 @@ import 'package:test/test.dart'; Future main() async { late final Directory directory; - late final FakeGithubApi fakeGithubApi; + late final FakeGithubApi Function(List additional) fakeGithubApi; setUpAll(() async { directory = Directory(p.join('test_data', 'test_repo')); - fakeGithubApi = FakeGithubApi(prLabels: [], files: [ - GitFile( - 'pkgs/package1/bin/package1.dart', - FileStatus.modified, - directory, - ), - GitFile( - 'pkgs/package2/lib/anotherLib.dart', - FileStatus.added, - directory, - ), - GitFile( - 'pkgs/package2/someImage.png', - FileStatus.added, - directory, - ), - GitFile( - 'pkgs/package5/lib/src/package5_base.dart', - FileStatus.modified, - directory, - ), - GitFile( - 'pkgs/package5/pubspec.yaml', - FileStatus.modified, - directory, - ), - ]); + fakeGithubApi = + (List additional) => FakeGithubApi(prLabels: [], files: [ + GitFile( + 'pkgs/package1/bin/package1.dart', + FileStatus.modified, + directory, + ), + GitFile( + 'pkgs/package2/lib/anotherLib.dart', + FileStatus.added, + directory, + ), + GitFile( + 'pkgs/package2/someImage.png', + FileStatus.added, + directory, + ), + GitFile( + 'pkgs/package5/lib/src/package5_base.dart', + FileStatus.modified, + directory, + ), + GitFile( + 'pkgs/package5/pubspec.yaml', + FileStatus.modified, + directory, + ), + ...additional + ]); await Process.run('dart', ['pub', 'global', 'activate', 'dart_apitool']); await Process.run('dart', ['pub', 'global', 'activate', 'coverage']); @@ -53,7 +56,28 @@ Future main() async { for (var check in Check.values) { test( 'Check health workflow "${check.name}" against golden files', - () async => await checkGolden(check, fakeGithubApi, directory), + () async => await checkGolden( + check, + fakeGithubApi([]), + directory, + ), + timeout: const Timeout(Duration(minutes: 2)), + ); + + test( + 'Check health workflow "${check.name}" against golden files ' + 'with health.yaml changed itself', + () async => await checkGolden( + check, + fakeGithubApi([ + GitFile( + '.github/workflows/health.yaml', + FileStatus.added, + directory, + ), + ]), + directory, + suffix: '_healthchanged'), timeout: const Timeout(Duration(minutes: 2)), ); } @@ -61,7 +85,7 @@ Future main() async { test('Ignore license test', () async { await checkGolden( Check.license, - fakeGithubApi, + fakeGithubApi([]), directory, suffix: '_ignore_license', ignoredLicense: ['pkgs/package3/**'], @@ -74,7 +98,7 @@ Future main() async { for (var check in Check.values) { await checkGolden( check, - fakeGithubApi, + fakeGithubApi([]), directory, suffix: '_ignore_package', ignoredPackage: ['pkgs/package1'], @@ -92,10 +116,11 @@ Future checkGolden( String suffix = '', List ignoredLicense = const [], List ignoredPackage = const [], + List flutterPackages = const [], }) async { final commentPath = p.join( Directory.systemTemp.createTempSync().path, 'comment_${check.name}.md'); - await Health( + await FakeHealth( directory, check, [], @@ -106,6 +131,7 @@ Future checkGolden( [], [], fakeGithubApi, + flutterPackages, base: Directory(p.join('test_data', 'base_test_repo')), comment: commentPath, log: printOnFailure, @@ -120,6 +146,29 @@ Future checkGolden( } } +class FakeHealth extends Health { + FakeHealth( + super.directory, + super.check, + super.warnOn, + super.failOn, + super.coverageweb, + super.ignoredPackages, + super.ignoredLicense, + super.ignoredCoverage, + super.experiments, + super.github, + super.flutterPackages, { + super.base, + super.comment, + super.log, + }); + + @override + String getCurrentVersionOfPackage(Package package) => + p.join('../base_test_repo/pkgs', package.name); +} + class FakeGithubApi implements GithubApi { final List files; diff --git a/pkgs/firehose/test_data/golden/comment_breaking_healthchanged.md b/pkgs/firehose/test_data/golden/comment_breaking_healthchanged.md new file mode 100644 index 00000000..62ca9598 --- /dev/null +++ b/pkgs/firehose/test_data/golden/comment_breaking_healthchanged.md @@ -0,0 +1,16 @@ +
+ +Breaking changes :warning: + + +| Package | Change | Current Version | New Version | Needed Version | Looking good? | +| :--- | :--- | ---: | ---: | ---: | ---: | +|package1|None|1.0.0|1.0.0|1.0.0|:heavy_check_mark:| +|package2|Non-Breaking|1.0.0|1.0.0|**1.1.0**
Got "1.0.0" expected >= "1.1.0" (non-breaking changes)|:warning:| +|package3|None|1.0.0|1.0.0|1.0.0|:heavy_check_mark:| +|package5|Non-Breaking|1.0.0|1.2.0|1.1.0|:heavy_check_mark:| + + +This check can be disabled by tagging the PR with `skip-breaking-check`. +
+ diff --git a/pkgs/firehose/test_data/golden/comment_changelog_healthchanged.md b/pkgs/firehose/test_data/golden/comment_changelog_healthchanged.md new file mode 100644 index 00000000..ae601aeb --- /dev/null +++ b/pkgs/firehose/test_data/golden/comment_changelog_healthchanged.md @@ -0,0 +1,16 @@ +
+ +Changelog Entry :exclamation: + + +| Package | Changed Files | +| :--- | :--- | +| package:package1 | pkgs/package1/bin/package1.dart | +| package:package2 | pkgs/package2/lib/anotherLib.dart | + +Changes to files need to be [accounted for](https://github.com/dart-lang/ecosystem/wiki/Changelog) in their respective changelogs. + + +This check can be disabled by tagging the PR with `skip-changelog-check`. +
+ diff --git a/pkgs/firehose/test_data/golden/comment_coverage_healthchanged.md b/pkgs/firehose/test_data/golden/comment_coverage_healthchanged.md new file mode 100644 index 00000000..5de5ebe3 --- /dev/null +++ b/pkgs/firehose/test_data/golden/comment_coverage_healthchanged.md @@ -0,0 +1,23 @@ +
+ +Coverage :warning: + + +| File | Coverage | +| :--- | :--- | +|pkgs/package1/bin/package1.dart| :broken_heart: Not covered | +|pkgs/package1/lib/package1.dart| :green_heart: 100 % | +|pkgs/package2/bin/package2.dart| :broken_heart: Not covered | +|pkgs/package2/lib/anotherLib.dart| :green_heart: 100 % | +|pkgs/package2/lib/package2.dart| :green_heart: 100 % | +|pkgs/package3/bin/package3.dart| :broken_heart: Not covered | +|pkgs/package3/lib/package3.dart| :green_heart: 100 % | +|pkgs/package5/lib/package5.dart| :broken_heart: Not covered | +|pkgs/package5/lib/src/package5_base.dart| :broken_heart: Not covered | + +This check for [test coverage](https://github.com/dart-lang/ecosystem/wiki/Test-Coverage) is informational (issues shown here will not fail the PR). + + +This check can be disabled by tagging the PR with `skip-coverage-check`. +
+ diff --git a/pkgs/firehose/test_data/golden/comment_do-not-submit_healthchanged.md b/pkgs/firehose/test_data/golden/comment_do-not-submit_healthchanged.md new file mode 100644 index 00000000..530c94f3 --- /dev/null +++ b/pkgs/firehose/test_data/golden/comment_do-not-submit_healthchanged.md @@ -0,0 +1,15 @@ +
+ +Do Not Submit :exclamation: + + +Body contains `DO_NOT_SUBMIT`: false + +| Files with `DO_NOT_SUBMIT` | +| :--- | +|pkgs/package1/bin/package1.dart| + + +This check can be disabled by tagging the PR with `skip-do-not-submit-check`. +
+ diff --git a/pkgs/firehose/test_data/golden/comment_leaking_healthchanged.md b/pkgs/firehose/test_data/golden/comment_leaking_healthchanged.md new file mode 100644 index 00000000..b0abc3d2 --- /dev/null +++ b/pkgs/firehose/test_data/golden/comment_leaking_healthchanged.md @@ -0,0 +1,15 @@ +
+ +API leaks :warning: + + +The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API. + +| Package | Leaked API symbols | +| :--- | :--- | +|package5|NonExported
NonExported2
TransitiveNonExported| + + +This check can be disabled by tagging the PR with `skip-leaking-check`. +
+ diff --git a/pkgs/firehose/test_data/golden/comment_license_healthchanged.md b/pkgs/firehose/test_data/golden/comment_license_healthchanged.md new file mode 100644 index 00000000..16e04cb8 --- /dev/null +++ b/pkgs/firehose/test_data/golden/comment_license_healthchanged.md @@ -0,0 +1,34 @@ +
+ +License Headers :exclamation: + + +``` +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +``` + +| Files | +| :--- | +|pkgs/package1/bin/package1.dart| +|pkgs/package1/lib/package1.dart| +|pkgs/package1/test/package1_test.dart| +|pkgs/package2/lib/anotherLib.dart| +|pkgs/package2/lib/package2.dart| +|pkgs/package2/test/package2_test.dart| +|pkgs/package3/bin/package3.dart| +|pkgs/package3/lib/package3.dart| +|pkgs/package3/test/package3_test.dart| +|pkgs/package5/lib/package5.dart| +|pkgs/package5/lib/src/package5_base.dart| + +All source files should start with a [license header](https://github.com/dart-lang/ecosystem/wiki/License-Header). + + + + + +This check can be disabled by tagging the PR with `skip-license-check`. +
+