Add more tests for @example, @youtube, and @animation. (#2298)

srawlins · web-flow · commit b4849726d793 · 2020-08-17T09:02:27.000-07:00
* Add more tests for @example, @youtube, and @animation. Additionally, make the ArgParsers for those directives static. Reduce crashes by adding proper returns. Move the error about missing @example file to the Package.warn system.
diff --git a/lib/src/model/comment_processable.dart b/lib/src/model/comment_processable.dart
@@ -2,12 +2,11 @@ import 'dart:io';
 
 import 'package:args/args.dart';
 import 'package:crypto/crypto.dart' as crypto;
-import 'package:dartdoc/src/logging.dart';
 import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/render/model_element_renderer.dart';
 import 'package:dartdoc/src/utils.dart';
 import 'package:dartdoc/src/warnings.dart';
-
+import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 
 final _templatePattern = RegExp(
@@ -75,7 +74,8 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
       // Remember, periods are legal in library names.
       fullyQualifiedName.replaceFirst('${library.fullyQualifiedName}.', '');
 
-  ModelElementRenderer get _modelElementRenderer =>
+  @visibleForTesting
+  ModelElementRenderer get modelElementRenderer =>
       packageGraph.rendererFactory.modelElementRenderer;
 
   /// Replace &#123;@tool ...&#125&#123;@end-tool&#125; in API comments with the
@@ -205,26 +205,28 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
           replacement = replacement.replaceFirst('```', '```$lang');
         }
       } else {
-        // TODO(jcollins-g): move this to Package.warn system
         var filePath = element.source.fullName.substring(dirPath.length + 1);
 
-        logWarning(
-            'warning: ${filePath}: @example file not found, ${fragmentFile.path}');
+        warn(PackageWarning.missingExampleFile,
+            message: '${fragmentFile.path}; path listed at $filePath');
       }
       return replacement;
     });
   }
 
+  /// An argument parser used in [_getExampleArgs] to parse an `{@example}`
+  /// directive.
+  static final ArgParser _exampleArgParser = ArgParser()
+    ..addOption('lang')
+    ..addOption('region');
+
   /// Helper for [_injectExamples] used to process `@example` arguments.
   ///
   /// Returns a map of arguments. The first unnamed argument will have key
   /// 'src'. The computed file path, constructed from 'src' and 'region' will
   /// have key 'file'.
   Map<String, String> _getExampleArgs(String argsAsString) {
-    var parser = ArgParser();
-    parser.addOption('lang');
-    parser.addOption('region');
-    var results = _parseArgs(argsAsString, parser, 'example');
+    var results = _parseArgs(argsAsString, _exampleArgParser, 'example');
     if (results == null) {
       return null;
     }
@@ -264,6 +266,10 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
   static final _validYouTubeUrlPattern =
       RegExp('https://www\.youtube\.com/watch\\?v=([^&]+)\$');
 
+  /// An argument parser used in [_injectYouTube] to parse a `{@youtube}`
+  /// directive.
+  static final _youTubeArgParser = ArgParser();
+
   /// Replace &#123;@youtube ...&#125; in API comments with some HTML to embed
   /// a YouTube video.
   ///
@@ -289,8 +295,7 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
   /// found in the address bar of the browser when viewing a YouTube video.
   String _injectYouTube(String rawDocs) {
     return rawDocs.replaceAllMapped(_basicYouTubePattern, (basicMatch) {
-      var parser = ArgParser();
-      var args = _parseArgs(basicMatch[1], parser, 'youtube');
+      var args = _parseArgs(basicMatch[1], _youTubeArgParser, 'youtube');
       if (args == null) {
         // Already warned about an invalid parameter if this happens.
         return '';
@@ -310,6 +315,7 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
             message: 'A @youtube directive has an invalid width, '
                 '"${positionalArgs[0]}". The width must be a positive '
                 'integer.');
+        return '';
       }
 
       var height = int.tryParse(positionalArgs[1]);
@@ -318,6 +324,7 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
             message: 'A @youtube directive has an invalid height, '
                 '"${positionalArgs[1]}". The height must be a positive '
                 'integer.');
+        return '';
       }
 
       var url = _validYouTubeUrlPattern.firstMatch(positionalArgs[2]);
@@ -332,7 +339,7 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
       var youTubeId = url.group(url.groupCount);
       var aspectRatio = (height / width * 100).toStringAsFixed(2);
 
-      return _modelElementRenderer.renderYoutubeUrl(youTubeId, aspectRatio);
+      return modelElementRenderer.renderYoutubeUrl(youTubeId, aspectRatio);
     });
   }
 
@@ -344,6 +351,10 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
   /// Matches valid javascript identifiers.
   final _validIdPattern = RegExp(r'^[a-zA-Z_]\w*$');
 
+  /// An argument parser used in [_injectAnimations] to parse a `{@animation}`
+  /// directive.
+  static final _animationArgParser = ArgParser()..addOption('id');
+
   /// Replace &#123;@animation ...&#125; in API comments with some HTML to
   /// manage an MPEG 4 video as an animation.
   ///
@@ -382,9 +393,7 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
       // Make sure we have a set to keep track of used IDs for this href.
       package.usedAnimationIdsByHref[href] ??= {};
 
-      var parser = ArgParser();
-      parser.addOption('id');
-      var args = _parseArgs(basicMatch[1], parser, 'animation');
+      var args = _parseArgs(basicMatch[1], _animationArgParser, 'animation');
       if (args == null) {
         // Already warned about an invalid parameter if this happens.
         return '';
@@ -463,7 +472,7 @@ mixin CommentProcessable on Documentable, Warnable, Locatable, SourceCodeMixin {
                 'parameter)');
       }
 
-      return _modelElementRenderer.renderAnimation(
+      return modelElementRenderer.renderAnimation(
           uniqueId, width, height, movieUrl, overlayId);
     });
   }
diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart
@@ -420,6 +420,9 @@ class PackageGraph {
       case PackageWarning.missingConstantConstructor:
         warningMessage = 'constant constructor missing: ${message}';
         break;
+      case PackageWarning.missingExampleFile:
+        warningMessage = 'example file not found: ${message}';
+        break;
     }
 
     var messageParts = <String>[warningMessage];
diff --git a/lib/src/warnings.dart b/lib/src/warnings.dart
@@ -263,6 +263,7 @@ enum PackageWarning {
   deprecated,
   unresolvedExport,
   missingConstantConstructor,
+  missingExampleFile,
 }
 
 /// Used to declare defaults for a particular package warning.
diff --git a/test/dartdoc_integration_test.dart b/test/dartdoc_integration_test.dart
@@ -169,31 +169,6 @@ void main() {
           endsWith('dartdoc version: ${dartdocMeta.version}\n'));
     });
 
-    test('Check for sample code in examples', () async {
-      var output = StringBuffer();
-      var args = <String>[
-        dartdocPath,
-        '--include',
-        'ex',
-        '--no-include-source',
-        '--output',
-        tempDir.path
-      ];
-
-      await subprocessLauncher.runStreamed(Platform.resolvedExecutable, args,
-          workingDirectory: _testPackagePath,
-          perLine: (s) => output.writeln(s));
-
-      // Examples are reported as unfound because we (purposefully)
-      // did not use --example-path-prefix above.
-      final sep = '.'; // We don't care what the path separator character is
-      final firstUnfoundExample = RegExp('warning: lib${sep}example.dart: '
-          '@example file not found.*test_package${sep}dog${sep}food.md');
-      if (!output.toString().contains(firstUnfoundExample)) {
-        fail('Should warn about unfound @example files');
-      }
-    });
-
     test('Validate JSON output', () async {
       var args = <String>[
         dartdocPath,
diff --git a/test/model_special_cases_test.dart b/test/model_special_cases_test.dart
@@ -13,7 +13,6 @@ import 'dart:io';
 import 'package:dartdoc/dartdoc.dart';
 import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/special_elements.dart';
-import 'package:dartdoc/src/warnings.dart';
 import 'package:pub_semver/pub_semver.dart';
 import 'package:test/test.dart';
 
@@ -486,199 +485,4 @@ void main() {
           '${HTMLBASE_PLACEHOLDER}dart-async/dart-async-library.html');
     });
   });
-
-  group('YouTube Errors', () {
-    PackageGraph packageGraphErrors;
-    Class documentationErrors;
-    Method withYouTubeWrongParams;
-    Method withYouTubeBadWidth;
-    Method withYouTubeBadHeight;
-    Method withYouTubeInvalidUrl;
-    Method withYouTubeUrlWithAdditionalParameters;
-
-    setUpAll(() async {
-      packageGraphErrors = await utils.testPackageGraphErrors;
-      documentationErrors = packageGraphErrors.libraries
-          .firstWhere((lib) => lib.name == 'doc_errors')
-          .classes
-          .firstWhere((c) => c.name == 'DocumentationErrors')
-            ..documentation;
-      withYouTubeWrongParams = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withYouTubeWrongParams')
-            ..documentation;
-      withYouTubeBadWidth = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withYouTubeBadWidth')
-            ..documentation;
-      withYouTubeBadHeight = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withYouTubeBadHeight')
-            ..documentation;
-      withYouTubeInvalidUrl = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withYouTubeInvalidUrl')
-            ..documentation;
-      withYouTubeUrlWithAdditionalParameters = documentationErrors
-          .instanceMethods
-          .firstWhere((m) => m.name == 'withYouTubeUrlWithAdditionalParameters')
-            ..documentation;
-    });
-
-    test('warns on youtube video with missing parameters', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withYouTubeWrongParams,
-              PackageWarning.invalidParameter,
-              'Invalid @youtube directive, "{@youtube https://youtu.be/oHg5SJYRHA0}"\n'
-              'YouTube directives must be of the form "{@youtube WIDTH HEIGHT URL}"'),
-          isTrue);
-    });
-    test('warns on youtube video with non-integer width', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withYouTubeBadWidth,
-              PackageWarning.invalidParameter,
-              'A @youtube directive has an invalid width, "100px". The width '
-              'must be a positive integer.'),
-          isTrue);
-    });
-    test('warns on youtube video with non-integer height', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withYouTubeBadHeight,
-              PackageWarning.invalidParameter,
-              'A @youtube directive has an invalid height, "100px". The height '
-              'must be a positive integer.'),
-          isTrue);
-    });
-    test('warns on youtube video with invalid video URL', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withYouTubeInvalidUrl,
-              PackageWarning.invalidParameter,
-              'A @youtube directive has an invalid URL: '
-              '"http://host/path/to/video.mp4". Supported YouTube URLs have '
-              'the following format: '
-              'https://www.youtube.com/watch?v=oHg5SJYRHA0.'),
-          isTrue);
-    });
-    test('warns on youtube video with extra parameters in URL', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withYouTubeUrlWithAdditionalParameters,
-              PackageWarning.invalidParameter,
-              'A @youtube directive has an invalid URL: '
-              '"https://www.youtube.com/watch?v=yI-8QHpGIP4&list=PLjxrf2q8roU23XGwz3Km7sQZFTdB996iG&index=5". '
-              'Supported YouTube URLs have the following format: '
-              'https://www.youtube.com/watch?v=oHg5SJYRHA0.'),
-          isTrue);
-    });
-  });
-
-  group('Animation Errors', () {
-    PackageGraph packageGraphErrors;
-    Class documentationErrors;
-    Method withInvalidNamedAnimation;
-    Method withAnimationNonUnique;
-    Method withAnimationNonUniqueDeprecated;
-    Method withAnimationWrongParams;
-    Method withAnimationBadWidth;
-    Method withAnimationBadHeight;
-    Method withAnimationUnknownArg;
-
-    setUpAll(() async {
-      packageGraphErrors = await utils.testPackageGraphErrors;
-      documentationErrors = packageGraphErrors.libraries
-          .firstWhere((lib) => lib.name == 'doc_errors')
-          .classes
-          .firstWhere((c) => c.name == 'DocumentationErrors')
-            ..documentation;
-      withInvalidNamedAnimation = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withInvalidNamedAnimation')
-            ..documentation;
-      withAnimationNonUnique = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withAnimationNonUnique')
-            ..documentation;
-      withAnimationNonUniqueDeprecated = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withAnimationNonUniqueDeprecated')
-            ..documentation;
-      withAnimationWrongParams = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withAnimationWrongParams')
-            ..documentation;
-      withAnimationBadWidth = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withAnimationBadWidth')
-            ..documentation;
-      withAnimationBadHeight = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withAnimationBadHeight')
-            ..documentation;
-      withAnimationUnknownArg = documentationErrors.instanceMethods
-          .firstWhere((m) => m.name == 'withAnimationUnknownArg')
-            ..documentation;
-    });
-
-    test('warns with invalidly-named animation within the method documentation',
-        () async {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withInvalidNamedAnimation,
-              PackageWarning.invalidParameter,
-              'An animation has an invalid identifier, "2isNot-A-ValidName". '
-              'The identifier can only contain letters, numbers and '
-              'underscores, and must not begin with a number.'),
-          isTrue);
-    });
-    test('warns on a non-unique animation name within a method', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withAnimationNonUnique,
-              PackageWarning.invalidParameter,
-              'An animation has a non-unique identifier, "barHerderAnimation". '
-              'Animation identifiers must be unique.'),
-          isTrue);
-    });
-    test('warns on a non-unique animation name within a deprecated-form method',
-        () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withAnimationNonUniqueDeprecated,
-              PackageWarning.invalidParameter,
-              'An animation has a non-unique identifier, "fooHerderAnimation". '
-              'Animation identifiers must be unique.'),
-          isTrue);
-    });
-    test('warns on animation with missing parameters', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withAnimationWrongParams,
-              PackageWarning.invalidParameter,
-              'Invalid @animation directive, "{@animation http://host/path/to/video.mp4}"\n'
-              'Animation directives must be of the form "{@animation WIDTH '
-              'HEIGHT URL [id=ID]}"'),
-          isTrue);
-    });
-    test('warns on animation with non-integer width', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withAnimationBadWidth,
-              PackageWarning.invalidParameter,
-              'An animation has an invalid width (badWidthAnimation), "100px". '
-              'The width must be an integer.'),
-          isTrue);
-    });
-    test('warns on animation with non-integer height', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withAnimationBadHeight,
-              PackageWarning.invalidParameter,
-              'An animation has an invalid height (badHeightAnimation), '
-              '"100px". The height must be an integer.'),
-          isTrue);
-    });
-    test('Unknown arguments generate an error.', () {
-      expect(
-          packageGraphErrors.packageWarningCounter.hasWarning(
-              withAnimationUnknownArg,
-              PackageWarning.invalidParameter,
-              'The {@animation ...} directive was called with invalid '
-              'parameters. FormatException: Could not find an option named "name".'),
-          isTrue);
-    });
-  }, timeout: Timeout.factor(2));
 }
diff --git a/test/unit/comment_processable_test.dart b/test/unit/comment_processable_test.dart

Original file line number	Diff line number	Diff line change
`@@ -420,6 +420,9 @@ class PackageGraph {`
`420`	`420`	`case PackageWarning.missingConstantConstructor:`
`421`	`421`	`warningMessage = 'constant constructor missing: ${message}';`
`422`	`422`	`break;`
	`423`	`+ case PackageWarning.missingExampleFile:`
	`424`	`+ warningMessage = 'example file not found: ${message}';`
	`425`	`+ break;`
`423`	`426`	`}`
`424`	`427`
`425`	`428`	`var messageParts = <String>[warningMessage];`
Original file line number	Diff line number	Diff line change
`@@ -263,6 +263,7 @@ enum PackageWarning {`
`263`	`263`	`deprecated,`
`264`	`264`	`unresolvedExport,`
`265`	`265`	`missingConstantConstructor,`
	`266`	`+ missingExampleFile,`
`266`	`267`	`}`
`267`	`268`
`268`	`269`	`/// Used to declare defaults for a particular package warning.`