Added a check to the analyzer script to detect skipped tests.

[darrenaustin]

Part of the Skip test audit.

Adds a check to the analyzer that enforces a justification comment after a skip parameter in tests, as described in Tree hygiene.

If a skipped test doesn't include a comment on the same line justifying why it is skipped, it will be considered an error. The comment should contain either a link to a tracking github issue, or include [intended] with a description of why the test is not designed for the skip condition.

The failure output will look something like:

% dart --enable-asserts dev/bots/analyze.dart
▌12:23:36▐ STARTING ANALYSIS
▌12:23:36▐ runtimeType in toString...
▌12:23:36▐ debug mode instead of checked mode...
▌12:23:37▐ Unexpected binaries...
▌12:23:37▐ RUNNING: cd .; git ls-files -z
▌12:23:37▐ ELAPSED TIME: 0.025s for git ls-files -z in .
▌12:23:37▐ Trailing spaces...
▌12:23:38▐ Deprecations...
▌12:23:39▐ Skip test comments...
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/integration.shard/variable_expansion_windows_test.dart:20}: skip test without a justification comment.
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/integration.shard/overall_experience_test.dart:418}: skip test without a justification comment.
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/general.shard/cache_test.dart:97}: skip test without a justification comment.
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/general.shard/cache_test.dart:106}: skip test without a justification comment.
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/web.shard/vm_service_web_test.dart:65}: skip test without a justification comment.
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/general.shard/macos/xcode_validator_test.dart:72}: skip test without a justification comment.
/Users/darrenaustin/dev/flutter/packages/flutter_tools/test/general.shard/android/gradle_test.dart:706}: skip test without a justification comment.

See: https://github.com/flutter/flutter/wiki/Tree-hygiene#skipped-tests
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Also updated the technical debt calculation to ignore [intended] skipped tests as they will never be reenabled and shouldn't count towards out debt.

[darrenaustin]

Reviewers: the analyzer failures are because I am still waiting for #87925's tests to finish so I can land it and pull it into this PR.

[goderbauer]

LGTM

[gspencergoog]

What happens if I have a "skip" parameter in a test class (or any other class that is used in a test, for that matter)?

e.g.:

class TestFoo {
  TestFoo({Set<int> values, bool skip = false});
  factory TestFoo.skip() => TestFoo(values: {1,2,3}, skip: true);
}

Seems like you really want is to use the Dart AST parser: regexes are only going to get you so far, and this kind of parsing is not something they really work well for (nested complex structures).

[gspencergoog]

I mean, I know it seems like overkill a bit, but on the other hand, I'm sure we'll hit a point where someone (maybe not even in the Flutter code) calls a parameter "skip" and gets a headache.

Writing the parser for this isn't that bad, I just did one for the snippets tool so that the parsing would be more robust (since I was using RegExps a lot before), and I was surprised at how little code it actually took, especially since you're only looking for a single parameter on a single kind of element.

[darrenaustin]

Thanks for the suggestion. I hear ya, and did consider that as I am not a fan of regexp for parsing. I was reluctant to introduce an analyzer script dependency on the Dart AST parser (although maybe that isn't a big deal). In addition, it might be trickier to handle the 'comment on the same line' with the parser.

The regexp solution is simpler and while potentially flawed, doesn't seem to have uncovered any other use cases of a skip keyword parameter in *_test.dart files in the flutter repo. So I went with the pragmatic option.

That said, I will take a look at what would be involved with migrating this to a parser solution. I may still go forward with this solution in the mean time just to keep more unmarked skip tests creeping into the tree.

[gspencergoog]

(I sent this to you in email, then saw your response here):

Here's an example of an AST parser that would give you the character offsets of each "skip" parameter attached to a call with a name that starts with 'test' or is called 'group'. It is harder to find the attached comments. Well, it's pretty easy to find preceding comments (e.g. "/* preceding comment */ skip: true"), but finding an inline comment at the end of the same line, for example, is pretty hard in the AST parser (it doesn't have any concept of "lines"). I'd just take the character offset ranges and use that to look for comments on the same line using regexps.

I don't think that the dependency is a big deal: we already depend on it in other places in the repo, and it's not customer facing code.

import 'package:analyzer/dart/analysis/features.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:file/file.dart';
import 'package:file/local.dart';

void main(List<String> args) {
  const FileSystem fs = LocalFileSystem();
  final List<Skip> skips = getFileSkips(fs.file(args[0])).toList();
  skips.sort((Skip a, Skip b) => a.charOffset.compareTo(b.charOffset));
  skips.forEach(print);
}

class Skip extends Object {
  Skip(this.element);
  final NamedExpression element;
  int get charOffset => element.beginToken.charOffset;
  int get endCharOffset => element.endToken.charOffset + element.endToken.length;

  @override
  String toString() {
    return '$charOffset-$endCharOffset: $element';
  }
}

Iterable<Skip> getFileSkips(File file) {
  // You could use parseString here instead.
  final ParseStringResult parseResult = parseFile(
    featureSet: FeatureSet.latestLanguageVersion(),
    path: file.absolute.path,
  );
  final _SourceVisitor<CompilationUnit> visitor =
      _SourceVisitor<CompilationUnit>(file);
  visitor.visitCompilationUnit(parseResult.unit);
  return visitor.skips;
}

class _SourceVisitor<T> extends RecursiveAstVisitor<T> {
  _SourceVisitor(this.file) : skips = <Skip>{};

  final Set<Skip> skips;
  File file;

  static bool isTestMethod(String name) {
    return name.startsWith('test') || name == 'group';
  }

  @override
  T? visitMethodInvocation(MethodInvocation node) {
    if (isTestMethod(node.methodName.toString())) {
      for (final Expression argument in node.argumentList.arguments) {
        if (argument is NamedExpression && argument.name.label.name == 'skip') {
          skips.add(Skip(argument));
        }
      }
    }
    return super.visitMethodInvocation(node);
  }
}

[gspencergoog]

Also, there is a method called LineInfo.fromContent() in the analyzer that will give you the line number for an offset.