Simplify how the list of TestCases for each TestConfiguration is made.

- Make it synchronous. I did some benchmarking and the async didn't seem
  to make a measurable difference and made the code harder to follow.
- Fix a bunch of small-scale idiom things: "var", names, etc.
- Move TestCase.hash into TestFile since that lets us access it before
  we've created a TestCase.
- Remove dead code.
- Make it clearer which functions work with TestFiles and which with
  TestCases.
- Clarify the code that determines whether or not to enqueue a test.

The last point is the motivating one. Soon, I'll be extending this code
to take NNBD into account when determining which tests to skip, so I
wanted to clean it up some first.

Change-Id: I488ed6c7d2453535968ac43389a2fd8370ead57d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/116662
Commit-Queue: Bob Nystrom <rnystrom@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>

pkg/test_runner/lib/src/multitest.dart

@@ -69,7 +69,6 @@
 /// ddd //# 07: static type warning
 /// fff
 /// ```
-import "dart:async";
 import "dart:io";
 
 import "path.dart";
@@ -171,9 +170,9 @@ void _generateTestsFromMultitest(Path filePath, Map<String, String> tests,
 ///
 /// Writes the resulting tests to [outputDir] and returns a list of [TestFile]s
 /// for each of those generated tests.
-Future<List<TestFile>> splitMultitest(
+List<TestFile> splitMultitest(
     TestFile multitest, String outputDir, Path suiteDir,
-    {bool hotReload}) async {
+    {bool hotReload}) {
   // Each key in the map tests is a multitest tag or "none", and the texts of
   // the generated test is its value.
   var tests = <String, String>{};
@@ -186,7 +185,6 @@ Future<List<TestFile>> splitMultitest(
 
   // Copy all the relative imports of the multitest.
   var importsToCopy = _findAllRelativeImports(multitest.path);
-  var futureCopies = <Future>[];
   for (var relativeImport in importsToCopy) {
     var importPath = Path(relativeImport);
     // Make sure the target directory exists.
@@ -197,14 +195,11 @@ Future<List<TestFile>> splitMultitest(
 
     // Copy file. Because some test suites may be read-only, we don't
     // want to copy the permissions, so we create the copy by writing.
-    final source = File(sourceDir.join(importPath).toNativePath()).openRead();
-    final target = File(targetDir.join(importPath).toNativePath()).openWrite();
-    futureCopies.add(source.cast<List<int>>().pipe(target));
+    var contents =
+        File(sourceDir.join(importPath).toNativePath()).readAsBytesSync();
+    File(targetDir.join(importPath).toNativePath()).writeAsBytesSync(contents);
   }
 
-  // Wait until all imports are copied before scheduling test cases.
-  await Future.wait(futureCopies);
-
   var baseFilename = multitest.path.filenameWithoutExtension;
 
   var testFiles = <TestFile>[];

pkg/test_runner/lib/src/process_queue.dart

@@ -235,17 +235,12 @@ class TestCaseEnqueuer {
     // system, generate tests, and search test files for options.
     var testCache = <String, List<TestFile>>{};
 
-    var iterator = testSuites.iterator;
-    void enqueueNextSuite() {
-      if (!iterator.moveNext()) {
-        // We're finished with building the dependency graph.
-        graph.seal();
-      } else {
-        iterator.current.forEachTest(_newTest, testCache, enqueueNextSuite);
-      }
+    for (var suite in testSuites) {
+      suite.findTestCases(_add, testCache);
     }
 
-    enqueueNextSuite();
+    // We're finished with building the dependency graph.
+    graph.seal();
   }
 
   /// Adds a test case to the list of active test cases, and adds its commands
@@ -259,14 +254,14 @@ class TestCaseEnqueuer {
   /// command of the previous copy of the test case. This dependency is
   /// marked as a "timingDependency", so that it doesn't depend on the previous
   /// test completing successfully, just on it completing.
-  void _newTest(TestCase testCase) {
+  void _add(TestCase testCase) {
     Node<Command> lastNode;
     for (var i = 0; i < testCase.configuration.repeat; ++i) {
       if (i > 0) {
         testCase = testCase.indexedCopy(i);
       }
       remainingTestCases.add(testCase);
-      bool isFirstCommand = true;
+      var isFirstCommand = true;
       for (var command in testCase.commands) {
         // Make exactly *one* node in the dependency graph for every command.
         // This ensures that we never have two commands c1 and c2 in the graph
@@ -280,7 +275,7 @@ class TestCaseEnqueuer {
           command2node[command] = node;
           command2testCases[command] = <TestCase>[];
         }
-        // Keep mapping from command to all testCases that refer to it
+        // Keep mapping from command to all testCases that refer to it.
         command2testCases[command].add(testCase);
 
         lastNode = node;

pkg/test_runner/lib/src/test_case.dart

@@ -14,7 +14,6 @@ import 'command_output.dart';
 import 'configuration.dart';
 import 'output_log.dart';
 import 'process_queue.dart';
-import 'repository.dart';
 import 'test_file.dart';
 import 'utils.dart';
 
@@ -55,28 +54,20 @@ class TestCase extends UniqueObject {
 
   TestConfiguration configuration;
   String displayName;
-  int hash = 0;
   Set<Expectation> expectedOutcomes;
   final TestFile testFile;
 
   TestCase(this.displayName, this.commands, this.configuration,
       this.expectedOutcomes,
-      {TestFile testFile})
-      : testFile = testFile {
+      {this.testFile}) {
     // A test case should do something.
     assert(commands.isNotEmpty);
-
-    if (testFile != null) {
-      hash = (testFile.originPath?.relativeTo(Repository.dir)?.toString())
-          .hashCode;
-    }
   }
 
   TestCase indexedCopy(int index) {
     var newCommands = commands.map((c) => c.indexedCopy(index)).toList();
     return TestCase(displayName, newCommands, configuration, expectedOutcomes,
-        testFile: testFile)
-      ..hash = hash;
+        testFile: testFile);
   }
 
   bool get hasRuntimeError => testFile?.hasRuntimeError ?? false;

pkg/test_runner/lib/src/test_configurations.dart

@@ -116,7 +116,7 @@ Future testConfigurations(List<TestConfiguration> configurations) async {
     // If we specifically pass in a suite only run that.
     if (configuration.suiteDirectory != null) {
       var suitePath = Path(configuration.suiteDirectory);
-      testSuites.add(PKGTestSuite(configuration, suitePath));
+      testSuites.add(PackageTestSuite(configuration, suitePath));
     } else {
       for (var testSuiteDir in TEST_SUITE_DIRECTORIES) {
         var name = testSuiteDir.filename;

pkg/test_runner/lib/src/test_file.dart

@@ -66,8 +66,20 @@ abstract class _TestFileBase {
   /// static error reporting.
   bool get isStaticErrorTest => expectedErrors.isNotEmpty;
 
+  /// A hash code used to spread tests across shards.
+  int get shardHash {
+    // The VM C++ unit tests have a special fake TestFile with no suite
+    // directory or path. Don't crash in that case.
+    // TODO(rnystrom): Is there a cleaner solution? Should we use the C++ file
+    // as the path for the TestFile?
+    if (originPath == null) return 0;
+
+    return originPath.relativeTo(_suiteDirectory).toString().hashCode;
+  }
+
   _TestFileBase(this._suiteDirectory, this.path, this.expectedErrors) {
-    assert(path.isAbsolute);
+    // The VM C++ unit tests have a special fake TestFile with no path.
+    if (path != null) assert(path.isAbsolute);
   }
 
   /// The logical name of the test.