From a9919f8a039d510e1014e4bca9756d017d230a61 Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Wed, 26 Aug 2015 23:54:12 -0500 Subject: [PATCH] Coverage: check for lcov dependency, fail helpfully. --- .gitignore | 4 +- README.md | 7 ++ lib/src/platform_util/api.dart | 15 ++++ lib/src/platform_util/mock_platform_util.dart | 44 +++++++++++ lib/src/platform_util/platform_util.dart | 26 +++++++ .../platform_util/standard_platform_util.dart | 28 +++++++ lib/src/tasks/coverage/api.dart | 78 ++++++++++--------- lib/src/tasks/coverage/cli.dart | 27 ++++--- lib/src/tasks/coverage/exceptions.dart | 26 +++++++ lib/src/tasks/format/cli.dart | 6 +- lib/src/tasks/test/cli.dart | 6 +- lib/src/util.dart | 14 ---- lib/util.dart | 3 +- test/integration/coverage_test.dart | 29 ++++++- 14 files changed, 244 insertions(+), 69 deletions(-) create mode 100644 lib/src/platform_util/api.dart create mode 100644 lib/src/platform_util/mock_platform_util.dart create mode 100644 lib/src/platform_util/platform_util.dart create mode 100644 lib/src/platform_util/standard_platform_util.dart create mode 100644 lib/src/tasks/coverage/exceptions.dart diff --git a/.gitignore b/.gitignore index 654e680f..f67505ec 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,6 @@ packages pubspec.lock -coverage/ \ No newline at end of file +/coverage/ +/test/fixtures/coverage/browser/coverage/ +/test/fixtures/coverage/vm/coverage/ \ No newline at end of file diff --git a/README.md b/README.md index a9b8b590..54e22dbb 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,13 @@ Name | Type | Default | Description `output` | `String` | `coverage/` | Output directory for coverage artifacts. `reportOn` | `List` | `['lib/']` | List of paths to include in the generated coverage report (LCOV and HTML). +> Note: "lcov" must be installed in order to generate the HTML report. +> +> If you're using brew, you can install it with: +> `brew update && brew install lcov` +> +> Otherwise, visit http://ltp.sourceforge.net/coverage/lcov.php + ### `examples` Config All configuration options for the `examples` task are found on the `config.examples` object. diff --git a/lib/src/platform_util/api.dart b/lib/src/platform_util/api.dart new file mode 100644 index 00000000..2fecd166 --- /dev/null +++ b/lib/src/platform_util/api.dart @@ -0,0 +1,15 @@ +library dart_dev.src.platform_util.api; + +import 'dart:async'; + +import 'package:dart_dev/src/platform_util/platform_util.dart'; + +/// Determines whether or not the project in the current working directory has +/// defined [packageName] as an immediate dependency. In other words, this +/// checks if [packageName] is in the project's pubspec.yaml. +bool hasImmediateDependency(String packageName) => + PlatformUtil.retrieve().hasImmediateDependency(packageName); + +/// Determines whether or not [executable] is installed on this platform. +Future isExecutableInstalled(String executable) => + PlatformUtil.retrieve().isExecutableInstalled(executable); diff --git a/lib/src/platform_util/mock_platform_util.dart b/lib/src/platform_util/mock_platform_util.dart new file mode 100644 index 00000000..51ae89be --- /dev/null +++ b/lib/src/platform_util/mock_platform_util.dart @@ -0,0 +1,44 @@ +library dart_dev.src.platform_util.mock_platform_util; + +import 'dart:async'; + +import 'package:dart_dev/src/platform_util/platform_util.dart'; +import 'package:dart_dev/src/platform_util/standard_platform_util.dart'; + +const List _defaultInstalledExecutables = const ['lcov']; + +const Map _defaultProjectDependencies = const { + 'coverage': '^0.7.2', + 'dart_style': '^0.2.0', + 'test': '^0.12.0' +}; + +class MockPlatformUtil implements PlatformUtil { + /// List of executables installed on this platform. Does not actually need + /// to be exhaustive, only needs to cover the executables that may be checked + /// by a dart_dev task. + static List installedExecutables = + _defaultInstalledExecutables.toList(); + + /// Map of dependencies that are defined by the current project. This + /// effectively mocks out any platform util that checks the pubspec.yaml + /// for dependencies. + static Map projectDependencies = + new Map.from(_defaultProjectDependencies); + + static void install() { + platformUtil = new MockPlatformUtil(); + } + + static void uninstall() { + platformUtil = new StandardPlatformUtil(); + installedExecutables = _defaultInstalledExecutables.toList(); + projectDependencies = new Map.from(_defaultProjectDependencies); + } + + bool hasImmediateDependency(String packageName) => + projectDependencies.containsKey(packageName); + + Future isExecutableInstalled(String executable) async => + installedExecutables.contains(executable); +} diff --git a/lib/src/platform_util/platform_util.dart b/lib/src/platform_util/platform_util.dart new file mode 100644 index 00000000..f32799b9 --- /dev/null +++ b/lib/src/platform_util/platform_util.dart @@ -0,0 +1,26 @@ +library dart_dev.src.platform_util.platform_util; + +import 'dart:async'; + +import 'package:dart_dev/src/platform_util/standard_platform_util.dart'; + +PlatformUtil platformUtil = new StandardPlatformUtil(); + +abstract class PlatformUtil { + static PlatformUtil retrieve() { + if (platformUtil == null) throw new StateError( + 'dart_dev\'s PlatformUtil instance must not be null.'); + return platformUtil; + } + + /// Generates an HTML report for an LCOV formatted coverage file. + // TODO: Future generateLcovHtml(String lcovPath, String outputPath); + + /// Determines whether or not the project in the current working directory has + /// defined [packageName] as an immediate dependency. In other words, this + /// checks if [packageName] is in the project's pubspec.yaml. + bool hasImmediateDependency(String packageName); + + /// Determines whether or not [executable] is installed on this platform. + Future isExecutableInstalled(String executable); +} diff --git a/lib/src/platform_util/standard_platform_util.dart b/lib/src/platform_util/standard_platform_util.dart new file mode 100644 index 00000000..db000040 --- /dev/null +++ b/lib/src/platform_util/standard_platform_util.dart @@ -0,0 +1,28 @@ +library dart_dev.src.platform_util.standard_platform_util; + +import 'dart:async'; +import 'dart:io'; + +import 'package:yaml/yaml.dart'; + +import 'package:dart_dev/src/platform_util/platform_util.dart'; + +class StandardPlatformUtil implements PlatformUtil { + bool hasImmediateDependency(String packageName) { + File pubspec = new File('pubspec.yaml'); + Map pubspecYaml = loadYaml(pubspec.readAsStringSync()); + List deps = []; + if (pubspecYaml.containsKey('dependencies')) { + deps.addAll((pubspecYaml['dependencies'] as Map).keys); + } + if (pubspecYaml.containsKey('dev_dependencies')) { + deps.addAll((pubspecYaml['dev_dependencies'] as Map).keys); + } + return deps.contains(packageName); + } + + Future isExecutableInstalled(String executable) async { + ProcessResult result = await Process.run('which', [executable]); + return result.exitCode == 0; + } +} diff --git a/lib/src/tasks/coverage/api.dart b/lib/src/tasks/coverage/api.dart index 889c293c..8cbedbb2 100644 --- a/lib/src/tasks/coverage/api.dart +++ b/lib/src/tasks/coverage/api.dart @@ -21,7 +21,9 @@ import 'dart:io'; import 'package:dart_dev/util.dart' show Reporter, TaskProcess, getOpenPort; import 'package:path/path.dart' as path; +import 'package:dart_dev/src/platform_util/api.dart' as platform_util; import 'package:dart_dev/src/tasks/coverage/config.dart'; +import 'package:dart_dev/src/tasks/coverage/exceptions.dart'; import 'package:dart_dev/src/tasks/task.dart'; const String _testFilePattern = '_test.dart'; @@ -67,16 +69,9 @@ class CoverageTask extends Task { String output: defaultOutput, List reportOn: defaultReportOn}) async { CoverageTask coverage = - new CoverageTask._(tests, html: html, output: output); - await coverage._collect(); - await coverage._format(reportOn); - - if (html) { - await coverage._generateReport(); - } - return new CoverageResult.success( - coverage.tests, coverage.collection, coverage.lcov, - report: coverage.report); + new CoverageTask._(tests, reportOn, html: html, output: output); + await coverage._run(); + return coverage.done; } /// Collect and format coverage for the given suite of [tests]. The @@ -96,22 +91,8 @@ class CoverageTask extends Task { String output: defaultOutput, List reportOn: defaultReportOn}) { CoverageTask coverage = - new CoverageTask._(tests, html: html, output: output); - - // Execute the coverage collection and formatting, but don't wait on it. - () async { - await coverage._collect(); - await coverage._format(reportOn); - - if (html) { - await coverage._generateReport(); - } - CoverageResult result = new CoverageResult.success( - coverage.tests, coverage.collection, coverage.lcov, - report: coverage.report); - coverage._done.complete(result); - }(); - + new CoverageTask._(tests, reportOn, html: html, output: output); + coverage._run(); return coverage; } @@ -135,6 +116,9 @@ class CoverageTask extends Task { /// searching all directories for valid test files. List _files = []; + /// Whether or not to generate the HTML report. + bool _html = defaultHtml; + /// File created to run the test in a browser. Need to store it so it can be /// cleaned up after the test finishes. File _lastHtmlFile; @@ -146,9 +130,14 @@ class CoverageTask extends Task { /// Directory to output all coverage related artifacts. Directory _outputDirectory; - CoverageTask._(List tests, + /// List of directories on which coverage should be reported. + List _reportOn; + + CoverageTask._(List tests, List reportOn, {bool html: defaultHtml, String output: defaultOutput}) - : _outputDirectory = new Directory(output) { + : _html = html, + _outputDirectory = new Directory(output), + _reportOn = reportOn { // Build the list of test files. tests.forEach((path) { if (path.endsWith(_testFilePattern) && @@ -212,7 +201,7 @@ class CoverageTask extends Task { // Run the test and obtain the observatory port for coverage collection. try { observatoryPort = await _test(_files[i]); - } on TestException { + } on CoverageTestSuiteException { _coverageErrorOutput.add('Tests failed: ${_files[i].path}'); continue; } @@ -244,7 +233,7 @@ class CoverageTask extends Task { _collection = _merge(collections); } - Future _format(List reportOn) async { + Future _format() async { _lcov = new File(path.join(_outputDirectory.path, 'coverage.lcov')); String executable = 'pub'; @@ -259,7 +248,7 @@ class CoverageTask extends Task { lcov.path, '--verbose' ]; - args.addAll(reportOn.map((p) => '--report-on=$p')); + args.addAll(_reportOn.map((p) => '--report-on=$p')); _coverageOutput.add(''); _coverageOutput.add('Formatting coverage'); @@ -323,6 +312,23 @@ class CoverageTask extends Task { return coverage; } + Future _run() async { + if (_html && !(await platform_util.isExecutableInstalled('lcov'))) { + _done.completeError(new MissingLcovException()); + return; + } + + await _collect(); + await _format(); + + if (_html) { + await _generateReport(); + } + + _done.complete( + new CoverageResult.success(tests, collection, lcov, report: report)); + } + Future _test(File file) async { // Look for a correlating HTML file. String htmlPath = file.absolute.path; @@ -419,14 +425,14 @@ class CoverageTask extends Task { await for (String line in process.stderr) { _coverageOutput.add(' $line'); if (line.contains(_observatoryFailPattern)) { - throw new TestException(); + throw new CoverageTestSuiteException(file.path); } if (line.contains(_observatoryPortPattern)) { Match m = _observatoryPortPattern.firstMatch(line); observatoryPort = int.parse(m.group(2)); } if (line.contains(_testsFailedPattern)) { - throw new TestException(); + throw new CoverageTestSuiteException(file.path); } if (line.contains(_testsPassedPattern)) { break; @@ -451,10 +457,10 @@ class CoverageTask extends Task { await for (String line in process.stdout) { _coverageOutput.add(' $line'); if (line.contains(_observatoryFailPattern)) { - throw new TestException(); + throw new CoverageTestSuiteException(file.path); } if (line.contains(_testsFailedPattern)) { - throw new TestException(); + throw new CoverageTestSuiteException(file.path); } if (line.contains(_testsPassedPattern)) { break; @@ -465,5 +471,3 @@ class CoverageTask extends Task { } } } - -class TestException implements Exception {} diff --git a/lib/src/tasks/coverage/cli.dart b/lib/src/tasks/coverage/cli.dart index 975e4b47..4dbc7a67 100644 --- a/lib/src/tasks/coverage/cli.dart +++ b/lib/src/tasks/coverage/cli.dart @@ -19,12 +19,14 @@ import 'dart:io'; import 'package:args/args.dart'; -import 'package:dart_dev/util.dart' show hasImmediateDependency, reporter; +import 'package:dart_dev/util.dart' show reporter; +import 'package:dart_dev/src/platform_util/api.dart' as platform_util; import 'package:dart_dev/src/tasks/cli.dart'; import 'package:dart_dev/src/tasks/config.dart'; import 'package:dart_dev/src/tasks/coverage/api.dart'; import 'package:dart_dev/src/tasks/coverage/config.dart'; +import 'package:dart_dev/src/tasks/coverage/exceptions.dart'; import 'package:dart_dev/src/tasks/test/config.dart'; class CoverageCli extends TaskCli { @@ -46,7 +48,8 @@ class CoverageCli extends TaskCli { final String command = 'coverage'; Future run(ArgResults parsedArgs) async { - if (!hasImmediateDependency('coverage')) return new CliResult.fail( + if (!platform_util + .hasImmediateDependency('coverage')) return new CliResult.fail( 'Package "coverage" must be an immediate dependency in order to run its executables.'); bool unit = parsedArgs['unit']; @@ -78,13 +81,19 @@ class CoverageCli extends TaskCli { } } - CoverageTask task = CoverageTask.start(tests, - html: html, - output: config.coverage.output, - reportOn: config.coverage.reportOn); - reporter.logGroup('Collecting coverage', - outputStream: task.output, errorStream: task.errorOutput); - CoverageResult result = await task.done; + CoverageResult result; + try { + CoverageTask task = CoverageTask.start(tests, + html: html, + output: config.coverage.output, + reportOn: config.coverage.reportOn); + reporter.logGroup('Collecting coverage', + outputStream: task.output, errorStream: task.errorOutput); + result = await task.done; + } on MissingLcovException catch (e) { + return new CliResult.fail(e.message); + } + if (result.successful && html && open) { Process.run('open', [result.reportIndex.path]); } diff --git a/lib/src/tasks/coverage/exceptions.dart b/lib/src/tasks/coverage/exceptions.dart new file mode 100644 index 00000000..9bb97c74 --- /dev/null +++ b/lib/src/tasks/coverage/exceptions.dart @@ -0,0 +1,26 @@ +library dart_dev.src.tasks.coverage.exceptions; + +const String missingLcovMessage = ''' +The "lcov" dependency is missing. It's required for generating the HTML report. + +If using brew, you can install it with: + brew update + brew install lcov + +Otherwise, visit http://ltp.sourceforge.net/coverage/lcov.php +'''; + +/// Thrown when collecting coverage on a test suite that has failing tests. +class CoverageTestSuiteException implements Exception { + final String message; + CoverageTestSuiteException(String testSuite) + : this.message = 'Test suite has failing tests: $testSuite'; + String toString() => 'CoverageTestSuiteException: $message'; +} + +/// Thrown when attempting to generate the HTML coverage report without the +/// required "lcov" dependency being installed. +class MissingLcovException implements Exception { + final String message = missingLcovMessage; + String toString() => 'MissingLcovException: $message'; +} diff --git a/lib/src/tasks/format/cli.dart b/lib/src/tasks/format/cli.dart index 06c1d3ae..98d1d99e 100644 --- a/lib/src/tasks/format/cli.dart +++ b/lib/src/tasks/format/cli.dart @@ -18,8 +18,9 @@ import 'dart:async'; import 'package:args/args.dart'; -import 'package:dart_dev/util.dart' show hasImmediateDependency, reporter; +import 'package:dart_dev/util.dart' show reporter; +import 'package:dart_dev/src/platform_util/api.dart' as platform_util; import 'package:dart_dev/src/tasks/format/api.dart'; import 'package:dart_dev/src/tasks/format/config.dart'; import 'package:dart_dev/src/tasks/cli.dart'; @@ -39,7 +40,8 @@ class FormatCli extends TaskCli { Future run(ArgResults parsedArgs) async { try { - if (!hasImmediateDependency('dart_style')) return new CliResult.fail( + if (!platform_util + .hasImmediateDependency('dart_style')) return new CliResult.fail( 'Package "dart_style" must be an immediate dependency in order to run its executables.'); } catch (e) { // It's possible that this check may throw if the pubspec.yaml diff --git a/lib/src/tasks/test/cli.dart b/lib/src/tasks/test/cli.dart index 79e15957..79d9d5ef 100644 --- a/lib/src/tasks/test/cli.dart +++ b/lib/src/tasks/test/cli.dart @@ -18,8 +18,9 @@ import 'dart:async'; import 'package:args/args.dart'; -import 'package:dart_dev/util.dart' show hasImmediateDependency, reporter; +import 'package:dart_dev/util.dart' show reporter; +import 'package:dart_dev/src/platform_util/api.dart' as platform_util; import 'package:dart_dev/src/tasks/cli.dart'; import 'package:dart_dev/src/tasks/config.dart'; import 'package:dart_dev/src/tasks/test/api.dart'; @@ -45,7 +46,8 @@ class TestCli extends TaskCli { final String command = 'test'; Future run(ArgResults parsedArgs) async { - if (!hasImmediateDependency('test')) return new CliResult.fail( + if (!platform_util + .hasImmediateDependency('test')) return new CliResult.fail( 'Package "test" must be an immediate dependency in order to run its executables.'); bool unit = parsedArgs['unit']; diff --git a/lib/src/util.dart b/lib/src/util.dart index 3e2ea45e..ae923b9e 100644 --- a/lib/src/util.dart +++ b/lib/src/util.dart @@ -18,7 +18,6 @@ import 'dart:async'; import 'dart:io'; import 'package:path/path.dart' as path; -import 'package:yaml/yaml.dart'; void copyDirectory(Directory source, Directory dest) { if (!dest.existsSync()) { @@ -65,19 +64,6 @@ Future getOpenPort() async { } } -bool hasImmediateDependency(String packageName) { - File pubspec = new File('pubspec.yaml'); - Map pubspecYaml = loadYaml(pubspec.readAsStringSync()); - List deps = []; - if (pubspecYaml.containsKey('dependencies')) { - deps.addAll((pubspecYaml['dependencies'] as Map).keys); - } - if (pubspecYaml.containsKey('dev_dependencies')) { - deps.addAll((pubspecYaml['dev_dependencies'] as Map).keys); - } - return deps.contains(packageName); -} - String parseExecutableFromCommand(String command) { return command.split(' ').first; } diff --git a/lib/util.dart b/lib/util.dart index 0924251f..16d38825 100644 --- a/lib/util.dart +++ b/lib/util.dart @@ -14,12 +14,13 @@ library dart_dev.util; +export 'package:dart_dev/src/platform_util/api.dart' + show hasImmediateDependency; export 'package:dart_dev/src/reporter.dart' show Reporter, reporter; export 'package:dart_dev/src/task_process.dart' show TaskProcess; export 'package:dart_dev/src/util.dart' show copyDirectory, getOpenPort, - hasImmediateDependency, parseArgsFromCommand, parseExecutableFromCommand; diff --git a/test/integration/coverage_test.dart b/test/integration/coverage_test.dart index d5cc658c..eef506ea 100644 --- a/test/integration/coverage_test.dart +++ b/test/integration/coverage_test.dart @@ -26,14 +26,15 @@ const String projectWithBrowserTests = 'test/fixtures/coverage/vm'; const String projectWithoutCoveragePackage = 'test/fixtures/coverage/no_coverage_package'; -Future runCoverage(String projectPath) async { +Future runCoverage(String projectPath, {bool html: false}) async { await Process.run('pub', ['get'], workingDirectory: projectPath); Directory oldCoverage = new Directory('$projectPath/coverage'); if (oldCoverage.existsSync()) { oldCoverage.deleteSync(recursive: true); } - List args = ['run', 'dart_dev', 'coverage', '--no-html']; + List args = ['run', 'dart_dev', 'coverage']; + args.add(html ? '--html' : '--no-html'); TaskProcess process = new TaskProcess('pub', args, workingDirectory: projectPath); @@ -55,8 +56,30 @@ void main() { expect(lcov.existsSync(), isTrue); }, timeout: new Timeout(new Duration(seconds: 60))); - test('should warn if "coverage" package is missing', () async { + test('should fail if "coverage" package is missing', () async { expect(await runCoverage(projectWithoutCoveragePackage), isFalse); }); + +// TODO: Will need to mock out the `genhtml` command as well. +// test('should not fail if "lcov" is installed and --html is set', () async { +// MockPlatformUtil.install(); +// expect(MockPlatformUtil.installedExecutables, contains('lcov')); +// expect(await runCoverage(projectWithVmTests, html: true), isTrue); +// MockPlatformUtil.uninstall(); +// }); + +// TODO: Will need to run coverage programmatically for these to work. See https://github.com/Workiva/dart_dev/issues/21 +// test('should fail if "lcov" is not installed and --html is set', () async { +// MockPlatformUtil.install(); +// MockPlatformUtil.installedExecutables.remove('lcov'); +// expect(await runCoverage(projectWithVmTests, html: true), isFalse); +// MockPlatformUtil.uninstall(); +// }); +// test('should not fail if "lcov" is not installed but --html is not set', () async { +// MockPlatformUtil.install(); +// MockPlatformUtil.installedExecutables.remove('lcov'); +// expect(await runCoverage(projectWithVmTests, html: false), isTrue); +// MockPlatformUtil.uninstall(); +// }); }); }