Fix bug in warnings where not specifying --show-warnings caused dartdoc to succeed even if there were errors.

Author: gspencergoog

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## 0.24.1-dev
 * Added more metadata (element name, project name, etc.) to external tool invocations.
+* Fixed a bug where not specifying --show-warnings caused dartdoc to report success
+  even when errors were present.
 
 ## 0.24.0
 * Add 'override' to feature list for members which override a superclass (#981)

lib/src/model.dart

@@ -6491,18 +6491,16 @@ class PackageBuilder {
     PackageWarningOptions warningOptions =
         new PackageWarningOptions(config.verboseWarnings);
     // TODO(jcollins-g): explode this into detailed command line options.
-    if (config.showWarnings) {
-      for (PackageWarning kind in PackageWarning.values) {
-        switch (kind) {
-          case PackageWarning.toolError:
-          case PackageWarning.invalidParameter:
-          case PackageWarning.unresolvedExport:
-            warningOptions.error(kind);
-            break;
-          default:
-            warningOptions.warn(kind);
-            break;
-        }
+    for (PackageWarning kind in PackageWarning.values) {
+      switch (kind) {
+        case PackageWarning.toolError:
+        case PackageWarning.invalidParameter:
+        case PackageWarning.unresolvedExport:
+          warningOptions.error(kind);
+          break;
+        default:
+          if (config.showWarnings) warningOptions.warn(kind);
+          break;
       }
     }
     return warningOptions;

lib/src/warnings.dart

@@ -101,7 +101,7 @@ final Map<PackageWarning, PackageWarningHelpText> packageWarningText = const {
   PackageWarning.unresolvedExport: const PackageWarningHelpText(
       PackageWarning.unresolvedExport,
       "unresolvedExport",
-      "An export refers to a URI that can not be resolved."),
+      "An export refers to a URI that cannot be resolved."),
 };
 
 /// Something that package warnings can be called on.  Optionally associated

test/dartdoc_test.dart

@@ -73,6 +73,45 @@ void main() {
         expect(Something.isPublic, isTrue);
         expect(Something.displayedCategories, isNotEmpty);
       });
+
+      test('errors generate errors even when warnings are off', () async {
+        Dartdoc dartdoc = await buildDartdoc([], testPackageToolError);
+        DartdocResults results = await dartdoc.generateDocsBase();
+        PackageGraph p = results.packageGraph;
+        Iterable<String> unresolvedToolErrors = p
+            .packageWarningCounter.countedWarnings.values
+            .expand<String>((Set<Tuple2<PackageWarning, String>> s) => s
+                .where((Tuple2<PackageWarning, String> t) =>
+                    t.item1 == PackageWarning.toolError)
+                .map<String>((Tuple2<PackageWarning, String> t) => t.item2));
+
+        expect(p.packageWarningCounter.errorCount, equals(1));
+        expect(unresolvedToolErrors.length, equals(1));
+        expect(unresolvedToolErrors.first,
+            contains('Tool "drill" returned non-zero exit code'));
+      });
+    });
+
+    group('Invoking command-line dartdoc', () {
+      String dartdocPath = pathLib.join('bin', 'dartdoc.dart');
+
+      test('errors cause non-zero exit when warnings are off', () async {
+        ProcessResult result = Process.runSync(Platform.resolvedExecutable, [
+          dartdocPath,
+          '--input=$testPackageToolError',
+          '--output=${pathLib.join(tempDir.absolute.path, 'test_package_tool_error')}',
+        ]);
+        expect(result.exitCode, isNonZero);
+      });
+      test('errors cause non-zero exit when warnings are on', () async {
+        ProcessResult result = Process.runSync(Platform.resolvedExecutable, [
+          dartdocPath,
+          '--input=$testPackageToolError',
+          '--output=${pathLib.join(tempDir.absolute.path, 'test_package_tool_error')}',
+          '--show-warnings',
+        ]);
+        expect(result.exitCode, isNonZero);
+      });
     });
 
     group('Option handling with cross-linking', () {

test/src/utils.dart

@@ -35,6 +35,8 @@ final Directory testPackageOptions =
     new Directory('testing/test_package_options');
 final Directory testPackageOptionsImporter =
     new Directory('testing/test_package_options_importer');
+final Directory testPackageToolError =
+    new Directory('testing/test_package_tool_error');
 
 /// Convenience factory to build a [DartdocGeneratorOptionContext] and associate
 /// it with a [DartdocOptionSet] based on the current working directory and/or

testing/test_package_tool_error/bin/drill.dart

@@ -0,0 +1,11 @@
+// Copyright (c) 2018, 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.
+
+// Used by tests as an "external tool" that always fails. Has no other useful purpose.
+
+import 'dart:io';
+
+void main(List<String> argList) {
+  exit(1);
+}

testing/test_package_tool_error/dartdoc_options.yaml

@@ -0,0 +1,5 @@
+dartdoc:
+  tools:
+    drill:
+      command: ["bin/drill.dart"]
+      description: "Puts holes in things."

testing/test_package_tool_error/lib/tool_user.dart

@@ -0,0 +1,8 @@
+abstract class ToolUser {
+  /// Invokes a tool that fails.
+  ///
+  /// {@tool drill}
+  /// Doesn't matter what's here: it's gonna fail.
+  /// {@end-tool}
+  void invokeFailingTool();
+}

testing/test_package_tool_error/pubspec.yaml

@@ -0,0 +1,5 @@
+name: test_package_tool_error
+version: 0.0.1
+description: A simple console application.
+environment:
+  sdk: <=3.0.0