Clean up test_runner Command subclass hash/equality.

tools/testing/dart/test_runner.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // 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.
 
 /**
  * Classes and methods for executing tests.
  *
  * This module includes:
  * - Managing parallel execution of tests, including timeout checks.
  * - Evaluating the output of each test as pass/fail/crash/timeout.
@@ skipping 30 matching lines @@
 const List<String> EXCLUDED_ENVIRONMENT_VARIABLES =
     const ['http_proxy', 'https_proxy', 'no_proxy',
            'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY'];
 
 
 /** A command executed as a step in a test case. */
 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;
 
   /** Reproduction command */
   String get reproductionCommand => null;
 
   // We compute the Command.hashCode lazily and cache it here, since it might
   // be expensive to compute (and hashCode is called often).
   int _cachedHashCode;
 
   Command._(this.displayName);
 
   int get hashCode {
     if (_cachedHashCode == null) {
       var builder = new HashCodeBuilder();
       _buildHashCode(builder);
       _cachedHashCode = builder.value;
     }
     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.addJson(displayName);
   }
 
-  void _buildHashCode(HashCodeBuilder builder) {
-    builder.add(commandLine);
-    builder.add(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;
 
   Future<bool> get outputIsUpToDate => new Future.value(false);
 }
 
 class ProcessCommand extends Command {
   /** Path to the executable of this command. */
   String executable;
 
@@ skipping 14 matching lines @@
     if (io.Platform.operatingSystem == 'windows') {
       // Windows can't handle the first command if it is a .bat file or the like
       // with the slashes going the other direction.
       // NOTE: Issue 1306
       executable = executable.replaceAll('/', '\\');
     }
   }
 
   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))
         .map(escapeCommandLineArgument).join(' ');
     if (workingDirectory != null) {
       command = "$command (working directory: $workingDirectory)";
     }
     return command;
   }
 
   Future<bool> get outputIsUpToDate => new Future.value(false);
 }
 
 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);
 
     Future<List<Uri>> readDepsFile(String path) {
       var file = new io.File(new Path(path).toNativePath());
       if (!file.existsSync()) {
         return new Future.value(null);
       }
       return file.readAsLines().then((List<String> lines) {
@@ skipping 24 matching lines @@
           }
           return true;
         }
       }
       return false;
     });
   }
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
-    builder.add(_outputFile);
-    builder.add(_neverSkipCompilation);
-    for (var uri in _bootstrapDependencies) builder.add(uri);
+    builder.addJson(_outputFile);
+    builder.addJson(_neverSkipCompilation);
+    builder.addJson(_bootstrapDependencies);
   }
 
-  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;
-  }
+  bool _equal(CompilationCommand other) =>
+      super._equal(other) &&
+      _outputFile == other._outputFile &&
+      _neverSkipCompilation == other._neverSkipCompilation &&
+      deepJsonCompare(_bootstrapDependencies, other._bootstrapDependencies);
 }
 
 class ContentShellCommand extends ProcessCommand {
   ContentShellCommand._(String executable,
                         String htmlFile,
                         List<String> options,
                         List<String> dartFlags,
                         Map<String, String> environmentOverrides)
       : super._("content_shell",
                executable,
                _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;
   }
 
   static List<String> _getArguments(List<String> options, String htmlFile) {
     var arguments = new List.from(options);
     arguments.add(htmlFile);
     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;
 }
 
 class BrowserTestCommand extends Command {
   final String browser;
   final String url;
   final Map configuration;
 
   BrowserTestCommand._(String _browser,
                        this.url,
                        this.configuration)
       : super._(_browser), browser = _browser;
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     builder.add(browser);
     builder.add(url);
     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(),
                 'tools/testing/dart/launch_browser.dart',
                 browser,
                 url];
     return parts.map(escapeCommandLineArgument).join(' ');
   }
 }
 
 class AnalysisCommand extends ProcessCommand {
   final String flavor;
 
   AnalysisCommand._(this.flavor,
                     String displayName,
                     String executable,
                     List<String> arguments,
                     Map<String, String> environmentOverrides)
       : super._(displayName, executable, arguments, environmentOverrides);
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     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 {
   VmCommand._(String executable,
               List<String> arguments,
               Map<String,String> environmentOverrides)
       : super._("vm", executable, arguments, environmentOverrides);
 }
 
 class JSCommandlineCommand extends ProcessCommand {
@@ skipping 18 matching lines @@
                 new io.File(pubExecutable).absolute.path,
                 [pubCommand],
                 {'PUB_CACHE' : pubCacheDirectory},
                 pubspecYamlDirectory), command = pubCommand;
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     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. */
 abstract class ScriptCommand extends Command {
   ScriptCommand._(String displayName) : super._(displayName);
 
   Future<ScriptCommandOutputImpl> run();
 }
 
 class CleanDirectoryCopyCommand extends ScriptCommand {
@@ skipping 30 matching lines @@
           this, Expectation.FAIL, "An error occured: $error.", watch.elapsed);
     });
   }
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     builder.add(_sourceDirectory);
     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 {
   String _pubspecYamlFile;
   String _destinationFile;
   Map<String, Map> _dependencyOverrides;
 
   ModifyPubspecYamlCommand._(this._pubspecYamlFile,
                              this._destinationFile,
                              this._dependencyOverrides)
@@ skipping 43 matching lines @@
       return new ScriptCommandOutputImpl(
           this, Expectation.PASS, "", watch.elapsed);
     }).catchError((error) {
       return new ScriptCommandOutputImpl(
           this, Expectation.FAIL, "An error occured: $error.", watch.elapsed);
     });
   }
 
   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);
 }
 
 /*
  * [MakeSymlinkCommand] makes a symbolic link to another directory.
  */
 class MakeSymlinkCommand extends ScriptCommand {
   String _link;
   String _target;
 
   MakeSymlinkCommand._(this._link, this._target) : super._('make_symlink');
@@ skipping 21 matching lines @@
           this, Expectation.FAIL, "An error occured: $error.", watch.elapsed);
     });
   }
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     builder.add(_link);
     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 {
   static final CommandBuilder instance = new CommandBuilder._();
 
   bool _cleared = false;
   final _cachedCommands = new Map<Command, Command>();
 
   CommandBuilder._();
 
@@ skipping 2380 matching lines @@
     }
   }
 
   void eventAllTestsDone() {
     for (var listener in _eventListener) {
       listener.allDone();
     }
     _allDone();
   }
 }