Clean up test_runner Command subclass hash/equality.

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
diff --git a/tools/testing/dart/test_runner.dart b/tools/testing/dart/test_runner.dart
index 8a13cdacae0b38de71063fa4026578125e102bbf..4e1a26355f17dcfdad2ca8eab9d11590dd14a5dc 100644
--- a/tools/testing/dart/test_runner.dart
+++ b/tools/testing/dart/test_runner.dart
@@ -48,9 +48,6 @@ class Command {
   /** A descriptive name for this command. */
   String displayName;
 
-  /** The actual command line that will be executed. */
-  String commandLine;
-
   /** Number of times this command *can* be retried */
   int get maxNumRetries => 2;
 
@@ -72,23 +69,16 @@ class Command {
     return _cachedHashCode;
   }
 
-  operator ==(other) {
-    if (other is Command) {
-      return identical(this, other) || _equal(other as Command);
-    }
-    return false;
-  }
+  operator ==(other) => identical(this, other) ||
+      (runtimeType == other.runtimeType && _equal(other));
 
   void _buildHashCode(HashCodeBuilder builder) {
-    builder.add(commandLine);
-    builder.add(displayName);
+    builder.addJson(displayName);
   }
 
-  bool _equal(Command other) {
-    return hashCode == other.hashCode &&
-        commandLine == other.commandLine &&
-        displayName == other.displayName;
-  }
+  bool _equal(Command other) =>
+      hashCode == other.hashCode &&
+      displayName == other.displayName;
 
   String toString() => reproductionCommand;
 
@@ -123,37 +113,18 @@ class ProcessCommand extends Command {
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
-    builder.add(executable);
-    builder.add(workingDirectory);
-    for (var object in arguments) builder.add(object);
-    if (environmentOverrides != null) {
-      for (var key in environmentOverrides.keys) {
-        builder.add(key);
-        builder.add(environmentOverrides[key]);
-      }
-    }
+    builder.addJson(executable);
+    builder.addJson(workingDirectory);
+    builder.addJson(arguments);
+    builder.addJson(environmentOverrides);
   }
 
-  bool _equal(Command other) {
-    if (other is ProcessCommand) {
-      if (!super._equal(other)) return false;
-
-      if (hashCode != other.hashCode ||
-          executable != other.executable ||
-          arguments.length != other.arguments.length) {
-        return false;
-      }
-
-      if (!deepJsonCompare(arguments, other.arguments)) return false;
-      if (workingDirectory != other.workingDirectory) return false;
-      if (!deepJsonCompare(environmentOverrides, other.environmentOverrides)) {
-        return false;
-      }
-
-      return true;
-    }
-    return false;
-  }
+  bool _equal(ProcessCommand other) =>
+      super._equal(other) &&
+      executable == other.executable &&
+      deepJsonCompare(arguments, other.arguments) &&
+      workingDirectory == other.workingDirectory &&
+      deepJsonCompare(environmentOverrides, other.environmentOverrides);
 
   String get reproductionCommand {
     var command = ([executable]..addAll(arguments))
@@ -168,22 +139,18 @@ class ProcessCommand extends Command {
 }
 
 class CompilationCommand extends ProcessCommand {
-  String _outputFile;
-  bool _neverSkipCompilation;
-  List<Uri> _bootstrapDependencies;
+  final String _outputFile;
+  final bool _neverSkipCompilation;
+  final List<Uri> _bootstrapDependencies;
 
   CompilationCommand._(String displayName,
                        this._outputFile,
                        this._neverSkipCompilation,
-                       List<Uri> bootstrapDependencies,
+                       this._bootstrapDependencies,
                        String executable,
                        List<String> arguments,
                        Map<String, String> environmentOverrides)
-      : super._(displayName, executable, arguments, environmentOverrides) {
-    // We sort here, so we can do a fast hashCode/operator==
-    _bootstrapDependencies = new List.from(bootstrapDependencies);
-    _bootstrapDependencies.sort();

[Bill Hesse, 2014/10/09 12:03:50]

All actual uses have only one entry, and if we make hashCode/== depend on order,
the only failure scenario is if two commands are completely identical except for
the order of this list, then we don't merge them, and there could be a race
condition, if they had the same target.   But how could two compilation commands
with the same target directory be made, that didn't follow the same code path
and have the same value for this.

We could also assert the list is sorted here, if we want to exclude this.  We
definitely don't want to make new copies every time.  We could also just make a
new copy if it isn't sorted, or cache the sorted version, but this seems to be
overkill.

-  }
+      : super._(displayName, executable, arguments, environmentOverrides);
 
   Future<bool> get outputIsUpToDate {
     if (_neverSkipCompilation) return new Future.value(false);
@@ -228,26 +195,16 @@ class CompilationCommand extends ProcessCommand {
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
-    builder.add(_outputFile);
-    builder.add(_neverSkipCompilation);
-    for (var uri in _bootstrapDependencies) builder.add(uri);
-  }
-
-  bool _equal(Command other) {
-    if (other is CompilationCommand &&
-        super._equal(other) &&
-        _outputFile == other._outputFile &&
-        _neverSkipCompilation == other._neverSkipCompilation &&
-        _bootstrapDependencies.length == other._bootstrapDependencies.length) {
-      for (var i = 0; i < _bootstrapDependencies.length; i++) {
-        if (_bootstrapDependencies[i] != other._bootstrapDependencies[i]) {
-          return false;
-        }
-      }
-      return true;
-    }
-    return false;
+    builder.addJson(_outputFile);
+    builder.addJson(_neverSkipCompilation);
+    builder.addJson(_bootstrapDependencies);
   }
+
+  bool _equal(CompilationCommand other) =>
+      super._equal(other) &&
+      _outputFile == other._outputFile &&
+      _neverSkipCompilation == other._neverSkipCompilation &&
+      deepJsonCompare(_bootstrapDependencies, other._bootstrapDependencies);
 }
 
 class ContentShellCommand extends ProcessCommand {
@@ -261,19 +218,27 @@ class ContentShellCommand extends ProcessCommand {
                _getArguments(options, htmlFile),
                _getEnvironment(environmentOverrides, dartFlags));
 
+  // Cache the modified environments in a map from the old environment and
+  // the string of Dart flags to the new environment.  Avoid creating new
+  // environment object for each command object.
+  static Map<Map, Map<String, Map>> _modifiedEnvironments =

[ricow1, 2014/10/09 12:16:48]

you may want to do a utility class for this, my mind at least can't understand
this easily

[Bill Hesse, 2014/10/10 11:59:15]

Done.  Use a key pair class, to uncurry the map to a map from a pair to a value,
rather than a map of maps.  Should be simpler.  I could have called the class
just "Pair", but gave it a specific name, and specific types.  We could wrap the
cached and returned environments in an UnmodifiableMapView, but I don't think it
is necessary.

+      new Map<Map, Map<String, Map>>();
+
   static Map _getEnvironment(Map<String, String> env, List<String> dartFlags) {
     var needDartFlags = dartFlags != null && dartFlags.length > 0;
-
     if (needDartFlags) {
-      if (env != null) {
-        env = new Map<String, String>.from(env);
-      } else {
-        env = new Map<String, String>();
+      if (env == null) {
+        env = const { };
       }
-      env['DART_FLAGS'] = dartFlags.join(" ");
-      env['DART_FORWARDING_PRINT'] = '1';
+      var flags = dartFlags.join(' ');
+      var envFromString =
+          _modifiedEnvironments.putIfAbsent(env, () => new Map<String, Map>());
+      return envFromString.putIfAbsent(flags, () =>
+          new Map<String, String>.from(env)..addAll({
+            'DART_FLAGS': flags,
+            'DART_FORWARDING_PRINT': '1'
+          }));
     }
-
     return env;
   }
 
@@ -283,9 +248,7 @@ class ContentShellCommand extends ProcessCommand {
     return arguments;
   }
 
-  bool _equal(Command other) {
-    return other is ContentShellCommand && super._equal(other);
-  }
+  bool _equal(Command other) => super._equal(other);  // We could omit this.

[ricow1, 2014/10/09 12:16:48]

so why don't we

[Bill Hesse, 2014/10/10 11:59:15]

Done.

 
   int get maxNumRetries => 3;
 }
@@ -307,14 +270,11 @@ class BrowserTestCommand extends Command {
     builder.add(configuration);
   }
 
-  bool _equal(Command other) {
-    return
-        other is BrowserTestCommand &&
-        super._equal(other) &&
-        browser == other.browser &&
-        url == other.url &&
-        identical(configuration, other.configuration);
-  }
+  bool _equal(BrowserTestCommand other) =>
+      super._equal(other) &&
+      browser == other.browser &&
+      url == other.url &&
+      identical(configuration, other.configuration);
 
   String get reproductionCommand {
     var parts = [TestUtils.dartTestExecutable.toString(),
@@ -340,12 +300,9 @@ class AnalysisCommand extends ProcessCommand {
     builder.add(flavor);
   }
 
-  bool _equal(Command other) {
-    return
-        other is AnalysisCommand &&
-        super._equal(other) &&
-        flavor == other.flavor;
-  }
+  bool _equal(AnalysisCommand other) =>
+      super._equal(other) &&
+      flavor == other.flavor;
 }
 
 class VmCommand extends ProcessCommand {
@@ -384,12 +341,9 @@ class PubCommand extends ProcessCommand {
     builder.add(command);
   }
 
-  bool _equal(Command other) {
-    return
-        other is PubCommand &&
-        super._equal(other) &&
-        command == other.command;
-  }
+  bool _equal(PubCommand other) =>
+      super._equal(other) &&
+      command == other.command;
 }
 
 /* [ScriptCommand]s are executed by dart code. */
@@ -440,13 +394,10 @@ class CleanDirectoryCopyCommand extends ScriptCommand {
     builder.add(_destinationDirectory);
   }
 
-  bool _equal(Command other) {
-    return
-        other is CleanDirectoryCopyCommand &&
-        super._equal(other) &&
-        _sourceDirectory == other._sourceDirectory &&
-        _destinationDirectory == other._destinationDirectory;
-  }
+  bool _equal(CleanDirectoryCopyCommand other) =>
+      super._equal(other) &&
+      _sourceDirectory == other._sourceDirectory &&
+      _destinationDirectory == other._destinationDirectory;
 }
 
 class ModifyPubspecYamlCommand extends ScriptCommand {
@@ -510,19 +461,16 @@ class ModifyPubspecYamlCommand extends ScriptCommand {
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
-    builder.add(_pubspecYamlFile);
-    builder.add(_destinationFile);
+    builder.addJson(_pubspecYamlFile);
+    builder.addJson(_destinationFile);
     builder.addJson(_dependencyOverrides);
   }
 
-  bool _equal(Command other) {
-    return
-        other is ModifyPubspecYamlCommand &&
-        super._equal(other) &&
-        _pubspecYamlFile == other._pubspecYamlFile &&
-        _destinationFile == other._destinationFile &&
-        deepJsonCompare(_dependencyOverrides, other._dependencyOverrides);
-  }
+  bool _equal(ModifyPubspecYamlCommand other) =>
+      super._equal(other) &&
+      _pubspecYamlFile == other._pubspecYamlFile &&
+      _destinationFile == other._destinationFile &&
+      deepJsonCompare(_dependencyOverrides, other._dependencyOverrides);
 }
 
 /*
@@ -564,13 +512,10 @@ class MakeSymlinkCommand extends ScriptCommand {
     builder.add(_target);
   }
 
-  bool _equal(Command other) {
-    return
-        other is MakeSymlinkCommand &&
-        super._equal(other) &&
-        _link == other._link &&
-        _target == other._target;
-  }
+  bool _equal(MakeSymlinkCommand other) =>
+      super._equal(other) &&
+      _link == other._link &&
+      _target == other._target;
 }
 
 class CommandBuilder {