Compare flutter warnings between versions very easily (#1600)

* Compare flutter warnings between versions very easily

* Add tests, clean up unused warning delta and its tests

* Add compare-flutter-warnings to contributing instructions

* dartfmt

Author: jcollins-g

CONTRIBUTING.md

@@ -24,7 +24,7 @@ yet in the issue tracker, start by opening an issue. Thanks!
 1. `grind` is needed to run dartdoc integration tests, see installed via `pub global activate grinder`.
 2. When a change is user-facing, please add a new entry to the [changelog](https://github.com/dart-lang/dartdoc/blob/master/CHANGELOG.md)
 3. Please include a test for your change.  `dartdoc` has both `package:test`-style unittests as well as integration tests.  To run the unittests, use `dart test/all.dart`.  Most changes can be tested via a unittest, but some require modifying the [test_package](https://github.com/dart-lang/dartdoc/tree/master/testing/test_package) and regenerating its docs via `grind update-test-package-docs`.
-4. For major changes, run `grind compare-sdk-warnings` and include the summary results in your pull request.
+4. For major changes, run `grind compare-sdk-warnings` and `grind compare-flutter-warnings`, and include the summary results in your pull request.
 5.  Be sure to format your Dart code using `dartfmt -w`, otherwise travis will complain.
 6.  Post your change via a pull request for review and integration!
 

lib/dartdoc.dart

@@ -107,7 +107,8 @@ class DartDoc extends PackageBuilder {
     // That seems wrong. dart-lang/dartdoc#1547
     for (Source source in sources) {
       ErrorsResult errorsResult = await driver.getErrors(source.fullName);
-      AnalysisErrorInfo info = new AnalysisErrorInfoImpl(errorsResult.errors, errorsResult.lineInfo);
+      AnalysisErrorInfo info =
+          new AnalysisErrorInfoImpl(errorsResult.errors, errorsResult.lineInfo);
       List<_Error> errors = [info]
           .expand((AnalysisErrorInfo info) {
             return info.errors.map((error) =>
@@ -133,7 +134,9 @@ class DartDoc extends PackageBuilder {
         })
         .where((_Error error) => error.isError)
         // TODO(jcollins-g): remove after conversion to analysis driver
-        .where((_Error error) => error.error.errorCode != CompileTimeErrorCode.URI_HAS_NOT_BEEN_GENERATED)
+        .where((_Error error) =>
+            error.error.errorCode !=
+            CompileTimeErrorCode.URI_HAS_NOT_BEEN_GENERATED)
         .toList()
           ..sort();
 

lib/src/io_utils.dart

@@ -88,8 +88,8 @@ class SubprocessLauncher {
     });
   }
 
-  SubprocessLauncher(this.context, [Map<String, String> environment]) :
-      this.environment = environment ?? <String, String>{};
+  SubprocessLauncher(this.context, [Map<String, String> environment])
+      : this.environment = environment ?? <String, String>{};
 
   /// A wrapper around start/await process.exitCode that will display the
   /// output of the executable continuously and fail on non-zero exit codes.
@@ -165,52 +165,3 @@ class SubprocessLauncher {
     return jsonObjects;
   }
 }
-
-/// Output formatter for comparing warnings.
-String printWarningDelta(String title, String dartdocOriginalBranch,
-    Map<String, int> original, Map<String, int> current) {
-  StringBuffer printBuffer = new StringBuffer();
-  Set<String> quantityChangedOuts = new Set();
-  Set<String> onlyOriginal = new Set();
-  Set<String> onlyCurrent = new Set();
-  Set<String> allKeys =
-      new Set.from([]..addAll(original.keys)..addAll(current.keys));
-
-  for (String key in allKeys) {
-    if (original.containsKey(key) && !current.containsKey(key)) {
-      onlyOriginal.add(key);
-    } else if (!original.containsKey(key) && current.containsKey(key)) {
-      onlyCurrent.add(key);
-    } else if (original.containsKey(key) &&
-        current.containsKey(key) &&
-        original[key] != current[key]) {
-      quantityChangedOuts.add(key);
-    }
-  }
-
-  if (onlyOriginal.isNotEmpty) {
-    printBuffer.writeln(
-        '*** $title : ${onlyOriginal.length} warnings from original ($dartdocOriginalBranch) missing in current:');
-    onlyOriginal.forEach((warning) => printBuffer.writeln(warning));
-  }
-  if (onlyCurrent.isNotEmpty) {
-    printBuffer.writeln(
-        '*** $title : ${onlyCurrent.length} new warnings not in original ($dartdocOriginalBranch)');
-    onlyCurrent.forEach((warning) => printBuffer.writeln(warning));
-  }
-  if (quantityChangedOuts.isNotEmpty) {
-    printBuffer.writeln('*** $title : Identical warning quantity changed');
-    for (String key in quantityChangedOuts) {
-      printBuffer.writeln(
-          "* Appeared ${original[key]} times in original ($dartdocOriginalBranch), now ${current[key]}:");
-      printBuffer.writeln(key);
-    }
-  }
-  if (onlyOriginal.isEmpty &&
-      onlyCurrent.isEmpty &&
-      quantityChangedOuts.isEmpty) {
-    printBuffer.writeln(
-        '*** $title : No difference in warning output from original ($dartdocOriginalBranch)${allKeys.isEmpty ? "" : " (${allKeys.length} warnings found)"}');
-  }
-  return printBuffer.toString();
-}

test/grind_test.dart

@@ -0,0 +1,82 @@
+// Copyright (c) 2015, 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.
+
+library dartdoc.io_utils_test;
+
+import 'package:test/test.dart';
+
+import '../tool/grind.dart' hide test;
+
+void main() {
+  group('printWarningDelta', () {
+    WarningsCollection original, current;
+    WarningsCollection originalWithDirs, currentWithDirs;
+    setUp(() {
+      original =
+          new WarningsCollection('/a/tempdir', '/pubcache/path', 'oldbranch');
+      original.add('originalwarning');
+      original.add('morewarning');
+      original.add('duplicateoriginalwarning');
+      original.add('duplicateoriginalwarning');
+      current =
+          new WarningsCollection('/a/tempdir2', '/pubcache/path2', 'current');
+      current.add('newwarning');
+      current.add('morewarning');
+      current.add('duplicateoriginalwarning');
+      originalWithDirs = new WarningsCollection(
+          '/a/tempdirFOO', '/pubcache/pathFOO', 'DirsOriginal');
+      originalWithDirs.add(
+          'originalWarning found in /a/tempdirFOO/some/subdir/program.dart!!!!');
+      originalWithDirs.add(
+          'another originalWarning found in /pubcache/pathFOO/some/package/lib/thingy.dart!!!');
+      originalWithDirs.add(
+          'insufficent exclamation mark warning found in /pubcache/pathFOO/some/package/lib/thingy.dart.');
+      originalWithDirs.add(
+          'another originalWarning found in /a/tempdirFOO/some/subdir/program.dart');
+      currentWithDirs = new WarningsCollection(
+          '/a/tempdirBAR', '/pubcache/pathBAR', 'DirsCurrent');
+      currentWithDirs.add(
+          'originalWarning found in /a/tempdirBAR/some/subdir/program.dart!!!!');
+      currentWithDirs.add(
+          'another originalWarning found in /pubcache/pathBAR/some/package/lib/thingy.dart!!!');
+      currentWithDirs.add(
+          'insufficent exclamation mark warning found in /pubcache/pathBAR/some/package/lib/thingy.dart.');
+      currentWithDirs.add(
+          'another originalWarning found in /a/tempdirBAR/some/other/subdir/program.dart');
+    });
+
+    test('verify that paths are substituted when comparing warnings', () {
+      expect(
+          originalWithDirs.getPrintableWarningDelta(
+              'Dirs diff title', currentWithDirs),
+          equals(
+              '*** Dirs diff title : 1 warnings from DirsOriginal, missing in DirsCurrent:\n'
+              'another originalWarning found in /a/tempdirFOO/some/subdir/program.dart\n'
+              '*** Dirs diff title : 1 new warnings in DirsCurrent, missing in DirsOriginal\n'
+              'another originalWarning found in /a/tempdirBAR/some/other/subdir/program.dart\n'
+              '*** Dirs diff title : Difference in warning output found for 2 warnings (5 warnings found)"\n'));
+    });
+
+    test('verify output of printWarningDelta', () {
+      expect(
+          original.getPrintableWarningDelta('Diff Title', current),
+          equals(
+              '*** Diff Title : 1 warnings from oldbranch, missing in current:\n'
+              'originalwarning\n'
+              '*** Diff Title : 1 new warnings in current, missing in oldbranch\n'
+              'newwarning\n'
+              '*** Diff Title : Identical warning quantity changed\n'
+              '* Appeared 2 times in oldbranch, 1 in current:\n'
+              'duplicateoriginalwarning\n'
+              '*** Diff Title : Difference in warning output found for 3 warnings (4 warnings found)"\n'));
+    });
+
+    test('verify output when nothing changes', () {
+      expect(
+          original.getPrintableWarningDelta('Diff Title 2', original),
+          equals(
+              '*** Diff Title 2 : No difference in warning output from oldbranch to oldbranch (3 warnings found)\n'));
+    });
+  });
+}

test/io_utils_test.dart

@@ -17,36 +17,4 @@ void main() {
       expect(getFileNameFor('dartdoc.generator'), 'dartdoc-generator.html');
     });
   });
-
-  group('printWarningDelta', () {
-    Map<String, int> original;
-    Map<String, int> current;
-    setUp(() {
-      original = new Map.fromIterables(
-          ["originalwarning", "morewarning", "duplicateoriginalwarning"],
-          [1, 1, 2]);
-      current = new Map.fromIterables(
-          ["newwarning", "morewarning", "duplicateoriginalwarning"], [1, 1, 1]);
-    });
-
-    test('verify output of printWarningDelta', () {
-      expect(
-          printWarningDelta('Diff Title', 'oldbranch', original, current),
-          equals(
-              '*** Diff Title : 1 warnings from original (oldbranch) missing in current:\n'
-              'originalwarning\n'
-              '*** Diff Title : 1 new warnings not in original (oldbranch)\n'
-              'newwarning\n'
-              '*** Diff Title : Identical warning quantity changed\n'
-              '* Appeared 2 times in original (oldbranch), now 1:\n'
-              'duplicateoriginalwarning\n'));
-    });
-
-    test('verify output when nothing changes', () {
-      expect(
-          printWarningDelta('Diff Title 2', 'oldbranch2', original, original),
-          equals(
-              '*** Diff Title 2 : No difference in warning output from original (oldbranch2) (3 warnings found)\n'));
-    });
-  });
 }

test/model_test.dart

@@ -320,10 +320,10 @@ void main() {
           dog.allInstanceMethods.firstWhere((m) => m.name == 'withMacro');
       withMacro2 =
           dog.allInstanceMethods.firstWhere((m) => m.name == 'withMacro2');
-      withPrivateMacro =
-          dog.allInstanceMethods.firstWhere((m) => m.name == 'withPrivateMacro');
-      withUndefinedMacro =
-          dog.allInstanceMethods.firstWhere((m) => m.name == 'withUndefinedMacro');
+      withPrivateMacro = dog.allInstanceMethods
+          .firstWhere((m) => m.name == 'withPrivateMacro');
+      withUndefinedMacro = dog.allInstanceMethods
+          .firstWhere((m) => m.name == 'withUndefinedMacro');
       package.allModelElements.forEach((m) => m.documentation);
     });
 
@@ -342,7 +342,10 @@ void main() {
     });
 
     test("a warning is generated for unknown macros", () {
-      expect(package.packageWarningCounter.hasWarning(withUndefinedMacro, PackageWarning.unknownMacro, 'ThatDoesNotExist'), isTrue);
+      expect(
+          package.packageWarningCounter.hasWarning(withUndefinedMacro,
+              PackageWarning.unknownMacro, 'ThatDoesNotExist'),
+          isTrue);
     });
   });
 
@@ -1908,7 +1911,8 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
       deprecated =
           exLibrary.constants.firstWhere((c) => c.name == 'deprecated');
       Class Dog = exLibrary.allClasses.firstWhere((c) => c.name == 'Dog');
-      customClassPrivate = fakeLibrary.constants.firstWhere((c) => c.name == 'CUSTOM_CLASS_PRIVATE');
+      customClassPrivate = fakeLibrary.constants
+          .firstWhere((c) => c.name == 'CUSTOM_CLASS_PRIVATE');
       aStaticConstField =
           Dog.allFields.firstWhere((f) => f.name == 'aStaticConstField');
       aName = Dog.allFields.firstWhere((f) => f.name == 'aName');
@@ -1957,7 +1961,8 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
     });
 
     test('MY_CAT is linked', () {
-      expect(cat.constantValue, 'const <a href="ex/ConstantCat/ConstantCat.html">ConstantCat</a>(&#39;tabby&#39;)');
+      expect(cat.constantValue,
+          'const <a href="ex/ConstantCat/ConstantCat.html">ConstantCat</a>(&#39;tabby&#39;)');
     });
 
     test('exported property', () {
@@ -2090,7 +2095,8 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
 
     test('linked return type', () {
       expect(t.linkedReturnType, equals('String'));
-      expect(generic.linkedReturnType, equals('List<span class="signature">&lt;S&gt;</span>'));
+      expect(generic.linkedReturnType,
+          equals('List<span class="signature">&lt;S&gt;</span>'));
     });
 
     test("name with generics", () {

test/src/utils.dart

@@ -42,7 +42,8 @@ init() async {
   testPackageGinormous = await bootBasicPackage(
       'testing/test_package', ['css', 'code_in_commnets', 'excluded'], true);
 
-  testPackageSmall = await bootBasicPackage('testing/test_package_small', [], false);
+  testPackageSmall =
+      await bootBasicPackage('testing/test_package_small', [], false);
   testPackageSdk = await bootSdkPackage();
 }