Added tools/status_clean.dart script

dart/tools/status_clean.dart

+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file

[ricow1, 2014/02/11 14:33:52]

2014

[kustermann, 2014/02/14 11:52:07]

Done.

+// 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 StatusFileParserTest;

[ricow1, 2014/02/11 14:33:52]

I find that name a little odd

[kustermann, 2014/02/14 11:52:07]

Done.

+
+import "dart:async";
+import "dart:convert" show JSON, UTF8;
+import "dart:io";
+import "testing/dart/status_file_parser.dart";
+import "testing/dart/multitest.dart";

[ricow1, 2014/02/11 14:33:52]

m<s

[kustermann, 2014/02/14 11:52:07]

Done.

+import "testing/dart/utils.dart" show Path;
+import "testing/dart/test_suite.dart"

[ricow1, 2014/02/11 14:33:52]

t<u

[kustermann, 2014/02/14 11:52:07]

Done.

+    show multiHtmlTestGroupRegExp, multiTestRegExp, multiHtmlTestRegExp,
+         TestUtils;
+
+// [STATUS_TUPLES] is a list of (suite-name, directory, status-file)-tuples.
+final STATUS_TUPLES = [
+    ["corelib", "tests/corelib", "tests/corelib/corelib.status"],
+    ["html", "tests/html", "tests/html/html.status"],
+    ["isolate", "tests/isolate", "tests/isolate/isolate.status"],
+    ["json", "tests/json", "tests/json/json.status"],
+    ["language", "tests/language", "tests/language/language.status"],
+    ["language", "tests/language", "tests/language/language_analyzer2.status"],
+    ["language","tests/language", "tests/language/language_analyzer.status"],
+    ["language","tests/language", "tests/language/language_dart2js.status"],
+    ["lib", "tests/lib", "tests/lib/lib.status"],
+    ["standalone", "tests/standalone", "tests/standalone/standalone.status"],
+    ["pkg", "pkg", "pkg/pkg.status"],
+
+    ["pkgbuild", ".", "pkg/pkgbuild.status"],
+    ["utils", "tests/utils", "tests/utils/utils.status"],
+    ["samples", "samples", "samples/samples.status"],
+    ["analyze_library", "sdk", "tests/lib/analyzer/analyze_library.status"],
+
+    ["dart2js_extra", "tests/compiler/dart2js_extra",
+     "tests/compiler/dart2js_extra/dart2js_extra.status"],
+    ["dart2js_native", "tests/compiler/dart2js_native",
+     "tests/compiler/dart2js_native/dart2js_native.status"],
+    ["dart2js", "tests/compiler/dart2js",
+     "tests/compiler/dart2js/dart2js.status"],
+
+    ["pub", "sdk/lib/_internal/pub", "sdk/lib/_internal/pub/pub.status"],
+    ["benchmark_smoke", "tests/benchmark_smoke",
+     "tests/benchmark_smoke/benchmark_smoke.status"],
+
+    ["co19", "tests/co19/src", "tests/co19/co19-analyzer2.status"],
+    ["co19", "tests/co19/src", "tests/co19/co19-analyzer.status"],
+    ["co19", "tests/co19/src", "tests/co19/co19-dart2dart.status"],
+    ["co19", "tests/co19/src", "tests/co19/co19-dart2js.status"],
+    ["co19", "tests/co19/src", "tests/co19/co19-co19.status"],
+    ["co19", "tests/co19/src", "tests/co19/co19-dartium.status"],
+    ["co19", "tests/co19/src", "tests/co19/co19-runtime.status"],
+];

[ricow1, 2014/02/11 14:33:52]

grouping (i.e., blank lines in the above seems randome

[kustermann, 2014/02/14 11:52:07]

The really bad thing about this is, that this information is not shared between
test.dart and status_clean. Adding a new status file requires two changes ....

Removed the blank lines.

+
+void main(List<String> args) {
+  usage() {
+    print("Usage: ${Platform.executable} <deflake|fix>");

[ricow1, 2014/02/11 14:33:52]

fix is not a very descriptive word here

[kustermann, 2014/02/14 11:52:07]

I'll change it to "remove-nonexistent-tests. Feel free to change it to something
better.

+    exit(1);
+  }
+
+  if (args.length == 0) usage();
+

[Bill Hesse, 2014/02/13 17:05:08]

You could extract the function to run (statusFileDeflaker.deflakeStatusFile or
invalidTestFixer.removeNonExistentTestsFromStatusFile) above the Future.forEach
loop, if the functions took the same arguments.

In other words, is it worth pulling the loops out of the conditional, and
merging them?

[kustermann, 2014/02/14 11:52:07]

I thought about it and left it like that because it was only 5 lines. I've
extracted it to a function and added a "run()" method to StatusFileProcessor.

+  if (args[0] == 'deflake') {
+    var statusFileDeflaker = new StatusFileDeflaker();
+    Future.forEach(STATUS_TUPLES, (List tuple) {
+      String suiteName = tuple[0];
+      String filePath = tuple[2];
+      print("Processing $filePath");
+      return statusFileDeflaker.deflakeStatusFile(suiteName, filePath);
+    });
+  } else if (args[0] == 'fix') {
+    var invalidTestFixer = new StatusFileNonExistentTestRemover();
+    Future.forEach(STATUS_TUPLES, (List tuple) {
+      String directory = tuple[1];
+      String filePath = tuple[2];
+      print("Processing $filePath");
+      return invalidTestFixer
+          .removeNonExistentTestsFromStatusFile(directory, filePath);
+    });
+  } else {
+    usage();
+  }
+}
+
+abstract class StatusFileProcessor {
+  Future<List<Section>> _readSections(String filePath) {
+    File file = new File(filePath);
+
+    if (file.existsSync()) {
+      var completer = new Completer();
+      List<Section> sections = new List<Section>();
+
+      ReadConfigurationInto(new Path(file.path), sections, () {
+        completer.complete(sections);
+      });
+      return completer.future;
+    }
+    return new Future.value([]);
+  }
+}
+
+class StatusFileNonExistentTestRemover extends StatusFileProcessor {
+  final MultiTestDetector multiTestDetector = new MultiTestDetector();
+  final TestFileLister testFileLister = new TestFileLister();
+
+  Future removeNonExistentTestsFromStatusFile(String directory,
+                                              String filePath) {
+    return _readSections(filePath).then((List<Section> sections) {
+      Set<int> invalidLines = _analyzeStatusFile(directory, filePath, sections);
+      if (invalidLines.length > 0) {
+        return _writeFixedStatusFile(filePath, invalidLines);
+      }
+      return new Future.value();
+    });
+  }
+
+  bool _doesTestExist(String filePath,

[Bill Hesse, 2014/02/13 17:05:08]

_testExists is a shorter name.

[kustermann, 2014/02/14 11:52:07]

Done.

+                      List<String> dartFiles,
+                      String directory,
+                      TestRule rule) {
+    List<RegExp> getRuleRegex(String name) {
+      return name.split("/")
+          .map((name) => new RegExp(name.replaceAll('*', '.*')))
+          .toList();
+    }
+    bool matchRegexp(List<RegExp> patterns, String str) {
+      var parts = str.split("/");
+      if (patterns.length > parts.length) {
+        return false;
+      }
+      // NOTE: patterns.length <= parts.length
+      for (var i = 0; i < patterns.length; i++) {
+        if (!patterns[i].hasMatch(parts[i])) {
+          return false;
+        }
+      }
+      return true;
+    }
+
+    if (rule.name.contains("packages") && filePath.contains("pkg.status")) {

[Bill Hesse, 2014/02/13 17:05:08]

Add a comment, that fix option does nothing to pkg.status?

[kustermann, 2014/02/14 11:52:07]

Well, that's not what it's doing. It's processing pkg.status as usual. The only
thing it's doing is that it ignores certain invalid patterns found in
pkg.status:
  8 */packages/*/*: Skip
  9 */*/packages/*/*: Skip
 10 */*/*/packages/*/*: Skip
 11 */*/*/*/packages/*/*: Skip 

Not all of these patterns match a testname. So my status_clean will report some
of these lines as invalid.

Now I actually think I shouldn't special case this, so I removed this construct.

+      return true;
+    }
+
+    var rulePattern = getRuleRegex(rule.name);
+    return dartFiles.any((String file) {
+      var relative = new Path(file).relativeTo(new Path(directory)).toString();
+      for (int splitIndex = 0; splitIndex < rulePattern.length; splitIndex++) {
+        // Construct a pattern for the file name.
+        var filePattern = new List();
+        for (var i = 0; i <= splitIndex; i++) {
+          filePattern.add(rulePattern[i]);
+        }
+
+        // Construct a pattern for the multitest name.
+        var multitestPattern = new List();
+        for (var i = splitIndex + 1; i < rulePattern.length; i++) {
+          multitestPattern.add(rulePattern[i]);
+        }
+
+        if (matchRegexp(filePattern, relative)) {

[Bill Hesse, 2014/02/13 17:05:08]

There is a strange semantics here, that filePattern could match an initial
segment of the string relative, and ignore extra parts, and then the
multitestPattern could match the multitest name of the multitest.  For example

foo/bar/can/dop.dart, containing a multitest  group1/test1, would match
foo/bar/group1/test1

I think it is clearer to find all the multitest names, join them to the file
names, and make a list of all complete test names in dartFiles, and just match
them against the unsplit rulePattern.

The relativeTo could be done outside the loop, since it is always the same.

+          // Could be a normal test
+          if (multitestPattern.length == 0) {
+            return true;
+          }
+
+          // Could be a real multitest.
+          if (multiTestDetector.getMultitestNamesFromFile(file).any(
+              (name) => matchRegexp(multitestPattern, name))) {
+            return true;
+          }
+
+          // Could be a multi html test.
+          if (multiTestDetector.getMultiHtmlTests(file).any(
+              (name) => matchRegexp(multitestPattern, name))) {
+            return true;
+          }
+        }
+      }
+      return false;
+    });
+  }
+
+  Set<int> _analyzeStatusFile(String directory,
+                              String filePath,
+                              List<Section> sections) {
+    var invalidLines = new Set<int>();
+    var dartFiles = testFileLister.listTestFiles(directory);
+    for (var section in sections) {
+      for (var rule in section.testRules) {
+        if (!_doesTestExist(filePath, dartFiles, directory, rule)) {
+          print("Invalid rule: ${rule.name} in file $filePath:${rule.lineNr}");
+          invalidLines.add(rule.lineNr);
+        }
+      }
+    }
+    return invalidLines;
+  }
+
+  _writeFixedStatusFile(String filePath, Set<int> invalidLines) {

[Bill Hesse, 2014/02/13 17:05:08]

statusFilePath

[kustermann, 2014/02/14 11:52:07]

Done.

+    var lines = new File(filePath).readAsLinesSync();
+    var outputLines = <String>[];
+    for (int i = 0; i < lines.length; i++) {
+      if (!invalidLines.contains(i + 1)) {

[Bill Hesse, 2014/02/13 17:05:08]

// The status file parser numbers lines starting with 1, not 0.

[kustermann, 2014/02/14 11:52:07]

Done.

+        outputLines.add(lines[i]);
+      }
+    }
+    var outputFile = new File("$filePath.fixed");
+    outputFile.writeAsStringSync(outputLines.join("\n"));

[Bill Hesse, 2014/02/13 17:05:08]

Writing the output can happen asynchronously, while the next status file is
processed:

outputFile.openWrite()..writeAll(outputLines, '\n')..close()

Similarly for deflaker.

[kustermann, 2014/02/14 11:52:07]

But then there is a future and nobody waits for it's completion. I find this
very strange behavior: We process a number of status files, the processing may
be done for some of them, but the result may only be half-written to disc.

This is really not a performance critical part. The critical piece is on the
server (which is currently extremely slow). I think it's fine to use sync IO
here.

+ }
+}
+
+class StatusFileDeflaker extends StatusFileProcessor {
+  TestOutcomeFetcher _testOutcomeFetcher = new TestOutcomeFetcher();
+
+  Future deflakeStatusFile(String suiteName, String filePath) {
+    return _readSections(filePath).then((List<Section> sections) {
+      var fixedLines = new Map<int, String>();

[Bill Hesse, 2014/02/13 17:05:08]

Dead variable, hidden by parameter name in closure.

[kustermann, 2014/02/14 11:52:07]

Dead and left-over variable :)
[I moved it into _generatedDeflakedLines and forgot to remove it here.]

+      return _generatedDeflakedLines(suiteName, sections)
+          .then((Map<int, String> fixedLines) {
+            if (fixedLines.length > 0) {
+              return _writeFixedStatusFile(filePath, fixedLines);
+            }
+      });
+    });
+  }
+
+  Future _generatedDeflakedLines(String suiteName,
+                                 List<Section> sections) {
+    var completer = new Completer();

[Bill Hesse, 2014/02/13 17:05:08]

Completer not needed.  Just 
 return Future.forEach(.....
 
 }.then((_) => fixedLines);

[kustermann, 2014/02/14 11:52:07]

Done.

+    var fixedLines = new Map<int, String>();
+    Future.forEach(sections, (Section section) {
+      return Future.forEach(section.testRules, (rule) {
+        return _maybeFixStatusfileLine(suiteName, section, rule, fixedLines);
+      });
+    }).then((_) => completer.complete(fixedLines));
+    return completer.future;
+  }
+
+  Future _maybeFixStatusfileLine(String suiteName,
+                                 Section section,
+                                 TestRule rule,
+                                 Map<int, String> fixedLines) {
+    print("Processing ${section.statusFile.location}: ${rule.lineNr}");
+    var notedOutcomes = rule.expression
+        .evaluate({})

[Bill Hesse, 2014/02/13 17:05:08]

Comment that none of our status files have expressions on their rules.

[kustermann, 2014/02/14 11:52:07]

Done.

+        .map((name) => Expectation.byName(name))
+        .where((Expectation expectation) => !expectation.isMetaExpectation)
+        .toSet();
+
+    if (notedOutcomes.isEmpty) return new Future.value();
+
+    return _testOutcomeFetcher.outcomesOf(suiteName, section, rule.name)
+      .then((Set<Expectation> actualOutcomes) {
+
+      var outcomesThatNeverHappend = new Set<Expectation>();

[Bill Hesse, 2014/02/13 17:05:08]

Happened

[kustermann, 2014/02/14 11:52:07]

Done.

+      for (Expectation notedOutcome in notedOutcomes) {
+        bool found = false;
+        for (Expectation actualOutcome in actualOutcomes) {
+          if (actualOutcome.canBeOutcomeOf(notedOutcome)) {
+            found = true;
+            break;
+          }
+        }
+        if (!found) {
+          outcomesThatNeverHappend.add(notedOutcome);
+        }
+      }
+
+      if (outcomesThatNeverHappend.length > 0 && actualOutcomes.length > 0) {
+        // Print the change to stdout.
+        print("${rule.name} (${section.statusFile.location}:${rule.lineNr}):");
+        print("   Actual outcomes:         ${actualOutcomes.toList()}");
+        print("   Outcomes in status file: ${notedOutcomes.toList()}");
+        print("   Outcomes in status file that never happened : "
+              "${outcomesThatNeverHappend.toList()}\n");
+
+        // Build the fixed status file line.
+        fixedLines[rule.lineNr] =
+            '${rule.name}: ${actualOutcomes.join(', ')} '
+            '# before: ${notedOutcomes.join(', ')} / '
+            'never happened:  ${outcomesThatNeverHappend.join(', ')}';
+      }
+    });
+  }
+
+  _writeFixedStatusFile(String filePath, Map<int, String> fixedLines) {
+    var lines = new File(filePath).readAsLinesSync();
+    var outputLines = <String>[];
+    for (int i = 0; i < lines.length; i++) {
+      if (fixedLines.containsKey(i + 1)) {
+        outputLines.add(fixedLines[i + 1]);
+      } else {
+        outputLines.add(lines[i]);
+      }
+    }
+    var output = outputLines.join("\n");
+    var outputFile = new File("$filePath.deflaked");
+    outputFile.writeAsStringSync(output);
+ }
+}
+
+class MultiTestDetector {
+  final multiTestsCache = new Map<String,List<String>>();
+  final multiHtmlTestsCache = new Map<String,List<String>>();
+
+  List<String> getMultitestNamesFromFile(String file) {
+    if (multiTestsCache.containsKey(file)) return multiTestsCache[file];
+
+    var tests = new Map<String, String>();
+    var outcomes = new Map<String, Set<String>>();
+    if (multiTestRegExp.hasMatch(new File(file).readAsStringSync())) {
+      ExtractTestsFromMultitest(new Path(file), tests, outcomes);
+    }
+    multiTestsCache[file] = tests.keys.toList();
+    return multiTestsCache[file];
+  }
+
+  List<String> getMultiHtmlTests(String file) {
+    if (multiHtmlTestsCache.containsKey(file)) return multiHtmlTestsCache[file];
+

[Bill Hesse, 2014/02/13 17:05:08]

return multiHtmlTestsCache.putIfAbsent(file, () {
...
})

[kustermann, 2014/02/14 11:52:07]

Done.

+    try {
+      List<String> subtestNames = [];
+      var content = new File(file).readAsStringSync();
+
+      if (multiHtmlTestRegExp.hasMatch(content)) {
+        var matchesIter = multiHtmlTestGroupRegExp.allMatches(content).iterator;
+        while(matchesIter.moveNext()) {
+          String fullMatch = matchesIter.current.group(0);
+          subtestNames.add(fullMatch.substring(fullMatch.indexOf("'") + 1));
+        }
+      }
+      multiHtmlTestsCache[file] = subtestNames;
+      return subtestNames;
+    } catch (e) {
+      print("WARNING: couldn't determine html multitests in file ${file}");
+    }
+    return [];
+  }
+}
+
+class TestFileLister {
+  final Map<String, List<String>> _filesCache = {};
+

[Bill Hesse, 2014/02/13 17:05:08]

putIfAbsent

[kustermann, 2014/02/14 11:52:07]

Done.

+  List<String> listTestFiles(String directory) {
+    if (_filesCache.containsKey(directory)) {
+      return _filesCache[directory];
+    }
+
+    var dir = new Directory(directory);
+    // Cannot test for _test.dart because co19 tests don't have that ending.
+    var dartFiles = dir.listSync(recursive: true)
+        .where((fe) => fe is File)
+        .where((file) => file.path.endsWith(".dart") ||
+                         file.path.endsWith("_test.html"))
+        .map((file) => file.path)
+        .toList();
+    _filesCache[directory] = dartFiles;
+    return dartFiles;
+  }
+}
+
+
+/*
+ * [TestOutcomeFetcher] will fetch test results from a server using a REST-like
+ * interface.
+ */
+class TestOutcomeFetcher {
+  static String SERVER = '108.170.219.8';

[Bill Hesse, 2014/02/13 17:05:08]

Is there anything better than hardcoding these values in here?

[kustermann, 2014/02/14 11:52:07]

No. We don't have a DNS name for it. I could make it a parameter of
status_clean.dart, but then we need to look the IP up every time we invoke this
script.

+  static int PORT = 4540;
+
+  HttpClient _client = new HttpClient();
+
+  Future<Set<Expectation>> outcomesOf(
+      String suiteName, Section section, String testName) {
+    var completer = new Completer();
+    var pathComponents = ['json', 'test-outcomes', 'outcomes',
+                          Uri.encodeComponent("$suiteName/$testName")];
+    var path = pathComponents.join('/') + '/';
+    var url = new Uri(scheme: 'http', host: SERVER, port: PORT, path: path);
+
+    _client.getUrl(url)
+      .then((HttpClientRequest request) => request.close())
+      .then((HttpClientResponse response) {
+        response
+            .transform(UTF8.decoder)
+            .transform(JSON.decoder).listen((List testResults) {

[Bill Hesse, 2014/02/13 17:05:08]

Is this .listen really a .first?  If so, then you can probably just return
setOfActualOutcomes from the successful case, and [] from the errors, and avoid
using a completer.

[kustermann, 2014/02/14 11:52:07]

Good point.

+              var setOfActualOutcomes = new Set<Expectation>();
+
+              try {
+                for (var result in testResults) {
+                  var config = result['configuration'];
+                  var testResult = result['test_result'];
+                  var outcome = testResult['outcome'];
+
+                  config['unchecked'] = !config['checked'];

[Bill Hesse, 2014/02/13 17:05:08]

Is there a way to keep these in sync with what the test scripts do?  A comment
referring to where these additions are in the test scripts?

[kustermann, 2014/02/14 11:52:07]

Added comment. I don't want to make this CL bigger by refactoring
test_options.dart as well.

+                  config['unminified'] = !config['minified'];
+                  config['nocsp'] = !config['csp'];
+                  config['browser'] =
+                      TestUtils.isBrowserRuntime(config['runtime']);
+                  config['analyzer'] =
+                      TestUtils.isCommandLineAnalyzer(config['compiler']);
+                  config['jscl'] =
+                      TestUtils.isJsCommandLineRuntime(config['runtime']);
+
+                  if (section.condition == null ||
+                      section.condition.evaluate(config)) {
+                    setOfActualOutcomes.add(Expectation.byName(outcome));
+                  }
+                }
+                completer.complete(setOfActualOutcomes);
+              } catch (error) {
+                print("Warning: Error occured while processing testoutcomes"
+                      ": $error");
+                completer.complete([]);
+              }
+        }, onError: (error) {
+          print("Warning: Error occured while fetching testoutcomes: $error");
+          completer.complete([]);
+        }, cancelOnError: true);
+    });
+    return completer.future;
+  }
+}