Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make health testable #224

Merged
merged 41 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
0c31c59
Add `ignore` flag to health workflows
mosuem Jan 8, 2024
6b487f8
Switch to glob
mosuem Jan 8, 2024
ee185bc
Fix multiline
mosuem Jan 8, 2024
7a9ab59
Add default glob
mosuem Jan 8, 2024
cbb7266
Delete multiline
mosuem Jan 8, 2024
9f84be4
Fix errors
mosuem Jan 8, 2024
6f1c834
Fix health commenting
mosuem Jan 8, 2024
128af77
Merge branch 'main' into addIgnoredFilesToHealth
mosuem Jan 12, 2024
9919dd0
Propagate ignore
mosuem Jan 12, 2024
c7073d5
Switch to specific ignores
mosuem Jan 12, 2024
2bacc71
Add defaults
mosuem Jan 12, 2024
4e15f4d
Add test repos
mosuem Jan 15, 2024
0962184
Start adding health tests
mosuem Jan 15, 2024
da5d59b
Add golden files
mosuem Jan 15, 2024
05e0dc9
Fix changelog
mosuem Jan 15, 2024
f623f41
Fix coverage
mosuem Jan 15, 2024
93a0d44
Fix breaking
mosuem Jan 15, 2024
989649e
More fixes
mosuem Jan 15, 2024
b201f0b
Revert "Add defaults"
mosuem Jan 15, 2024
1bfb902
Remove ignores
mosuem Jan 15, 2024
854a8c8
Remove ignores from yamls
mosuem Jan 15, 2024
554e975
Remove from firehose
mosuem Jan 15, 2024
f9f760d
Remove from github
mosuem Jan 15, 2024
ab836e8
Remove from repo
mosuem Jan 15, 2024
2ec6d84
Add changelog
mosuem Jan 15, 2024
464d11b
Move goldens
mosuem Jan 15, 2024
dfcb177
Fix glob issue
mosuem Jan 15, 2024
285edbd
Add test data to pubignore
mosuem Jan 15, 2024
95f2a62
Switch SDK version
mosuem Jan 15, 2024
e9b3465
Move stuff around
mosuem Jan 15, 2024
27e5f2a
Fix imports
mosuem Jan 15, 2024
5ccd5a5
Give test a name
mosuem Jan 15, 2024
ae6bac7
Sort license files
mosuem Jan 15, 2024
b46ff67
Switch debug messages
mosuem Jan 15, 2024
47ea57e
Add debug string
mosuem Jan 15, 2024
75551e9
Add debug
mosuem Jan 15, 2024
0ef7cb3
Add activate global
mosuem Jan 15, 2024
5916e77
Remove debugging
mosuem Jan 15, 2024
b8e0c88
Fix coverage upload
mosuem Jan 15, 2024
29c04b4
Merge branch 'main' into makeHealthTestable
mosuem Jan 16, 2024
6bad589
fix erros
mosuem Jan 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/health.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions .github/workflows/health_base.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -131,7 +130,7 @@ jobs:
if: ${{ '$action_state' == 1 }}

- name: Upload coverage to Coveralls
if: ${{ inputs.upload_coverage }}
if: ${{ inputs.upload_coverage && inputs.check == 'coverage' }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bugfix, before, coverage was always uploaded as upload_coverage defaults to true.

uses: coverallsapp/github-action@3dfc5567390f6fa9267c0ee9c251e4c8c3f18949
with:
format: lcov
Expand Down
1 change: 1 addition & 0 deletions pkgs/firehose/.pubignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_data/
4 changes: 4 additions & 0 deletions pkgs/firehose/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.6.0

- Make the health workflow testable with golden tests.

## 0.5.3

- Allow experiments to be enabled for Dart.
Expand Down
4 changes: 4 additions & 0 deletions pkgs/firehose/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
include: package:dart_flutter_team_lints/analysis_options.yaml

analyzer:
exclude:
- test_data/*
2 changes: 2 additions & 0 deletions pkgs/firehose/bin/health.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -51,5 +52,6 @@ void main(List<String> arguments) async {
failOn,
coverageWeb,
experiments,
GithubApi(),
).healthCheck();
}
2 changes: 1 addition & 1 deletion pkgs/firehose/lib/firehose.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
36 changes: 18 additions & 18 deletions pkgs/firehose/lib/src/github.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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';
Expand Down
39 changes: 23 additions & 16 deletions pkgs/firehose/lib/src/health/changelog.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
Expand Down
67 changes: 36 additions & 31 deletions pkgs/firehose/lib/src/health/coverage.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import 'dart:io';

import 'package:collection/collection.dart';
import 'package:path/path.dart' as path;

import '../github.dart';
Expand All @@ -14,39 +15,39 @@ import 'lcov.dart';

class Coverage {
final bool coverageWeb;
final Directory directory;
final List<String> experiments;

Coverage(this.coverageWeb, this.experiments);
Coverage(this.coverageWeb, this.directory, this.experiments);

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');
Expand All @@ -59,14 +60,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,
);
Expand All @@ -76,14 +78,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) {
Expand All @@ -103,7 +107,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',
[
Expand All @@ -119,7 +123,7 @@ 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',
[
Expand All @@ -130,8 +134,9 @@ Get coverage for ${package.name} by running coverage in ${package.directory.path
],
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',
[
Expand All @@ -149,8 +154,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,
Expand Down
Loading
Loading