Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21001003:
test.py: First step towards support of caching dart2js compilations across runtimes (Closed)
|
Created:
7 years, 4 months ago by kustermann Modified:
5 years, 7 months ago Reviewers:
ricow1 CC:
reviews_dartlang.org Visibility:
Public. |
Descriptiontest.py: First step towards support of caching dart2js compilations across runtimes
This CL changes the way we run tests completely: When enqueueing TestCases we
build up a dependency graph of Commands. This has the advantage that if more
TestCases share common commands, we only execute it once.
R=ricow@google.com
Committed: https://code.google.com/p/dart/source/detail?r=25760
Patch Set 1 #
Total comments: 83
Patch Set 2 : #
Total comments: 51
Patch Set 3 : #Patch Set 4 : rebased #
Total comments: 5
Messages
Total messages: 15 (1 generated)
Request for first round of comments on general approach.
Looks really good, I like the approach First round of comments: Add doc style comments for the graph class https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... File tools/testing/dart/dependency_graph.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:73: Timer.run(() { add a comment stating why we do this asynchronously https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:79: class Node extends UniqueObject { I like the fact that these instance variables are actually private - will eliminate the urge to do dirty hacks in the testing scripts https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:96: static NodeState Enqueing = new NodeState._("Enqueing"); Enqueing -> enqueuing https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:98: static NodeState Successfull = new NodeState._("Successfull"); Successfull -> Successful https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/record_and... File tools/testing/dart/record_and_replay.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/record_and... tools/testing/dart/record_and_replay.dart:101: var relative = new Path(rawArgument).relativeTo(new Path(_cwd)); I like it :-), but please add a comment stating why we do this https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_optio... File tools/testing/dart/test_options.dart (left): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_optio... tools/testing/dart/test_options.dart:583: if (configuration['valgrind']) { Could you file a bug to re-add this functionality and ask on vm list if anybody is using it? https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:61: /** Number of times this command could be retried */ could -> should ? https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:64: int _cachedHashCode; add comment what this is https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:107: if (executable == other.executable && you could do a fast return here if other.hashCode != hashCode, even better start with if (hashCode != other.hashCode || executable != other.executable || commandLine != other.commandLine || displayName != other.displayName || arguments.length != other.arguments.length) { return false; } https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:111: if ((environment == null && other.environment != null) || if ((environment != other.environment) && (environment == null || other.environment == null) https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:116: environment.length != other.environment.length) { this can go bad, other.environment may be null https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:119: for (var i=0; i<arguments.length; i++) { space around = and < https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:119: for (var i=0; i<arguments.length; i++) { why don't you move this up above line 111, then you can eliminate some checks and simplify environment chekcing to: if (other.environment == null && environment == null) return true; if (other.environment == null || environment == null) return false; if (other.environment.length != environment.lenght) return false; for (var key in environment.keys) { if (!other.environment.containsKey(key) || environment[key] != other.environment[key]) { return false; } } return true; https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:124: if (!other.environment.containsKey(key) || again, other.environment may be null https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:213: for (var i=0; i<_bootstrapDependencies.length; i++) { space around = and < https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:218: /* commented out code https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:239: String htmlFile, indentation https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:244: executable, indentation https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:348: this.flavour, String displayName, String executable, List<String> arguments) long line https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:367: final _cachedCommands = new Map<Command, Command>(); you could just use a HashSet here https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:414: var command = new AnalysisCommand._(flavour, displayName, executable, arguments); Long line https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:425: Command _getUniqueCommand(Command command) { please add comment stating how this works https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:528: /* commented out code https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:549: expectedOutcomes, isNegative: isNegative, info: info); indentation https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:609: // TODO(kustermann): Remove testCase from this class. I think you already did https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:672: //print("runtime error expected: compilation was ${exitCode == 0 ? "successfull " : "failed" }"); commented out long line :-) https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:701: // the test. we just show a warning here, we don't retry https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:850: // TODO(kustermann): Remove testCase from this class I think you did https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1183: Completer<CommandOutput> completer; why is this public when everything else is private - I don't see any uses externally https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1210: completer = new Completer<CommandOutput>(); we could eliminate this if you refactor doStartTest and reportResult to return futures instead, but I don't feel strongly about this https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1405: * It will maintain three helper data structures Extend this comment to say: The node structure is explicitly build to guarantee that objects with the same instance variables have the same hashCode and return true in the equality operator when the instance variables are the same https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1454: */ a lot of commented out code https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1458: //print("adding ${testCase.displayName}"); commented out code https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1537: bool allDependenciesSuccessful = node.dependencies.every( You could move this to the if body below https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1725: void runCommand(int retriesLeft) { why not remove the completer and do: Future runCommand(int retriesLeft) { return _runCommand(command, timeout).then((CommandOutput output) { if (!output.canRunDependendCommands && retriesLeft > 0) { DebugLogger.warning("Rerunning Command: ($retriesLeft " "attempt(s) remains) [cmd: $command]"); return runCommand(retriesLeft - 1); } else return output; } }); return runCommand(command.numRetries); https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1742: var completer = new Completer(); this is unused https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:2050: /* commented out https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_suite... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1639: var command = CommandBuilder.instance.getCommand('junit_test', 'java', args); long line https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:101: for (int i=startPos; i < (data.length-pattern.length); i++) { space around = I would remove parenthesis around data.length-pattern.length https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:103: for (int j=0; j<pattern.length; j++) { space around = and < https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:105: found = false; break here?
PTAL https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... File tools/testing/dart/dependency_graph.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:73: Timer.run(() { On 2013/07/30 09:30:11, ricow1 wrote: > add a comment stating why we do this asynchronously Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:79: class Node extends UniqueObject { On 2013/07/30 09:30:11, ricow1 wrote: > I like the fact that these instance variables are actually private - will > eliminate the urge to do dirty hacks in the testing scripts Yes. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:96: static NodeState Enqueing = new NodeState._("Enqueing"); On 2013/07/30 09:30:11, ricow1 wrote: > Enqueing -> enqueuing Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/dependency... tools/testing/dart/dependency_graph.dart:98: static NodeState Successfull = new NodeState._("Successfull"); On 2013/07/30 09:30:11, ricow1 wrote: > Successfull -> Successful Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/record_and... File tools/testing/dart/record_and_replay.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/record_and... tools/testing/dart/record_and_replay.dart:101: var relative = new Path(rawArgument).relativeTo(new Path(_cwd)); On 2013/07/30 09:30:11, ricow1 wrote: > I like it :-), but please add a comment stating why we do this Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:61: /** Number of times this command could be retried */ On 2013/07/30 09:30:11, ricow1 wrote: > could -> should ? should? No. Normally we don't want to retry anything. But we allow retrying this command numRetries times if it failed. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:64: int _cachedHashCode; On 2013/07/30 09:30:11, ricow1 wrote: > add comment what this is It's a cached hashcode! :-) Added comment. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:107: if (executable == other.executable && On 2013/07/30 09:30:11, ricow1 wrote: > you could do a fast return here if other.hashCode != hashCode, even better start > with > if (hashCode != other.hashCode || > executable != other.executable || > commandLine != other.commandLine || > displayName != other.displayName || > arguments.length != other.arguments.length) { > return false; > } Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:111: if ((environment == null && other.environment != null) || On 2013/07/30 09:30:11, ricow1 wrote: > if ((environment != other.environment) && > (environment == null || other.environment == null) Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:116: environment.length != other.environment.length) { On 2013/07/30 09:30:11, ricow1 wrote: > this can go bad, other.environment may be null No. The check above makes sure that either both environments are null (in which case the shortcut operator will not evaluate the ' && XXX') or both are '!= null'! https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:119: for (var i=0; i<arguments.length; i++) { On 2013/07/30 09:30:11, ricow1 wrote: > space around = and < Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:119: for (var i=0; i<arguments.length; i++) { On 2013/07/30 09:30:11, ricow1 wrote: > why don't you move this up above line 111, then you can eliminate some checks > and simplify environment chekcing to: > if (other.environment == null && environment == null) return true; > if (other.environment == null || environment == null) return false; > if (other.environment.length != environment.lenght) return false; > for (var key in environment.keys) { > if (!other.environment.containsKey(key) || > environment[key] != other.environment[key]) { > return false; > } > } > return true; I moved the expensive operations down (iterating through arguments/environment) and the fast operations up, so we can fail as fast as possible (not getting to the expensive operations) if the commands are not equal. I think it doesn't make a big difference though. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:124: if (!other.environment.containsKey(key) || On 2013/07/30 09:30:11, ricow1 wrote: > again, other.environment may be null Again, no. It can't. If one environment is null and the other is not, we return false above. So we are guaranteed that either of these two cases hold: env != null and other.env != null env == null and other.env == null The check on line 122 rules out option 2. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:213: for (var i=0; i<_bootstrapDependencies.length; i++) { On 2013/07/30 09:30:11, ricow1 wrote: > space around = and < Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:218: /* On 2013/07/30 09:30:11, ricow1 wrote: > commented out code Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:239: String htmlFile, On 2013/07/30 09:30:11, ricow1 wrote: > indentation Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:244: executable, On 2013/07/30 09:30:11, ricow1 wrote: > indentation Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:348: this.flavour, String displayName, String executable, List<String> arguments) On 2013/07/30 09:30:11, ricow1 wrote: > long line Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:367: final _cachedCommands = new Map<Command, Command>(); On 2013/07/30 09:30:11, ricow1 wrote: > you could just use a HashSet here By doing it this way, I do not only get equal commands but identical commands, see "_getUniqueCommand()". https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:414: var command = new AnalysisCommand._(flavour, displayName, executable, arguments); On 2013/07/30 09:30:11, ricow1 wrote: > Long line Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:425: Command _getUniqueCommand(Command command) { On 2013/07/30 09:30:11, ricow1 wrote: > please add comment stating how this works Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:528: /* On 2013/07/30 09:30:11, ricow1 wrote: > commented out code Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:549: expectedOutcomes, isNegative: isNegative, info: info); On 2013/07/30 09:30:11, ricow1 wrote: > indentation Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:609: // TODO(kustermann): Remove testCase from this class. On 2013/07/30 09:30:11, ricow1 wrote: > I think you already did Not completely, we still pass it in to let the CommandOutput object determine if it failed or not. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:672: //print("runtime error expected: compilation was ${exitCode == 0 ? "successfull " : "failed" }"); On 2013/07/30 09:30:11, ricow1 wrote: > commented out long line :-) Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:701: // the test. On 2013/07/30 09:30:11, ricow1 wrote: > we just show a warning here, we don't retry Yes, I removed the requestRetry side-effect from this class. If it really turns out to be an issue, I'll add it again. (I got rid of tons of special-casing/side-effects otherwise I would've spent too much time on these things rather than on the general approach). https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:850: // TODO(kustermann): Remove testCase from this class On 2013/07/30 09:30:11, ricow1 wrote: > I think you did Not completely. didFail() takes now a testCase as an argument. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1183: Completer<CommandOutput> completer; On 2013/07/30 09:30:11, ricow1 wrote: > why is this public when everything else is private - I don't see any uses > externally Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1210: completer = new Completer<CommandOutput>(); On 2013/07/30 09:30:11, ricow1 wrote: > we could eliminate this if you refactor doStartTest and reportResult to return > futures instead, but I don't feel strongly about this Yes, but let's keep it this way for now. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1405: * It will maintain three helper data structures On 2013/07/30 09:30:11, ricow1 wrote: > Extend this comment to say: > The node structure is explicitly build to guarantee that objects with the same > instance variables have the same hashCode and return true in the equality > operator when the instance variables are the same Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1454: */ On 2013/07/30 09:30:11, ricow1 wrote: > a lot of commented out code Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1458: //print("adding ${testCase.displayName}"); On 2013/07/30 09:30:11, ricow1 wrote: > commented out code Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1537: bool allDependenciesSuccessful = node.dependencies.every( On 2013/07/30 09:30:11, ricow1 wrote: > You could move this to the if body below Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1725: void runCommand(int retriesLeft) { On 2013/07/30 09:30:11, ricow1 wrote: > why not remove the completer and do: > Future runCommand(int retriesLeft) { > return _runCommand(command, timeout).then((CommandOutput output) { > if (!output.canRunDependendCommands && retriesLeft > 0) { > DebugLogger.warning("Rerunning Command: ($retriesLeft " > "attempt(s) remains) [cmd: $command]"); > return runCommand(retriesLeft - 1); > } else > return output; > } > }); > return runCommand(command.numRetries); Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1742: var completer = new Completer(); On 2013/07/30 09:30:11, ricow1 wrote: > this is unused Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:2050: /* On 2013/07/30 09:30:11, ricow1 wrote: > commented out Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_suite... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1639: var command = CommandBuilder.instance.getCommand('junit_test', 'java', args); On 2013/07/30 09:30:11, ricow1 wrote: > long line Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:101: for (int i=startPos; i < (data.length-pattern.length); i++) { On 2013/07/30 09:30:11, ricow1 wrote: > space around = > I would remove parenthesis around data.length-pattern.length Done. Kept the parenthesis (it's not that obvious that operator- binds stronger than operator<) https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:103: for (int j=0; j<pattern.length; j++) { On 2013/07/30 09:30:11, ricow1 wrote: > space around = and < Done. https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:105: found = false; On 2013/07/30 09:30:11, ricow1 wrote: > break here? Done.
I will take another closer look, but here are reponses and a style nits https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:701: // the test. On 2013/07/31 15:53:54, kustermann wrote: > On 2013/07/30 09:30:11, ricow1 wrote: > > we just show a warning here, we don't retry > > Yes, I removed the requestRetry side-effect from this class. > > If it really turns out to be an issue, I'll add it again. > (I got rid of tons of special-casing/side-effects otherwise I would've spent too > much time on these things rather than on the general approach). OK, my point was that the warning we print is now probably wrong, it says we will retry https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/1/tools/testing/dart/utils.dart... tools/testing/dart/utils.dart:101: for (int i=startPos; i < (data.length-pattern.length); i++) { On 2013/07/31 15:53:54, kustermann wrote: > On 2013/07/30 09:30:11, ricow1 wrote: > > space around = > > I would remove parenthesis around data.length-pattern.length > > Done. Kept the parenthesis (it's not that obvious that operator- binds stronger > than operator<) OH, as discussed offline I though length-pattern was an accessor :-) https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1357: //print("STARTING $executable ${arguments.join(' ')} "); commented out code https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1369: //print("STDOUT $line"); commented out line https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1393: //print("STDERR: $line"); commented out line https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/utils.... File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/utils.... tools/testing/dart/utils.dart:104: if (data[i+j] != pattern[j]) { space around +
Some more comments https://codereview.chromium.org/21001003/diff/24001/tests/standalone/io/test_... File tests/standalone/io/test_runner_test.dart (right): https://codereview.chromium.org/21001003/diff/24001/tests/standalone/io/test_... tests/standalone/io/test_runner_test.dart:80: _makeCrashTestCase(name, expectations) { where do call this from? https://codereview.chromium.org/21001003/diff/24001/tests/utils/dependency_gr... File tests/utils/dependency_graph_test.dart (right): https://codereview.chromium.org/21001003/diff/24001/tests/utils/dependency_gr... tests/utils/dependency_graph_test.dart:59: for (var i=0; i<events.length; i++) { could we indent this for loop 4 more spaces, this makes it hard to read https://codereview.chromium.org/21001003/diff/24001/tools/test.dart File tools/test.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/test.dart#newcode242 tools/test.dart:242: new ProcessQueue(firstConf, it seems a little odd to pass in the first conf, but I understand the incentive to do so. Could you add a small comment stating why we do that? https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/depend... File tools/testing/dart/dependency_graph.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/depend... tools/testing/dart/dependency_graph.dart:9: Since this is general purpose it would be beneficial to add doc style comments to this https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/depend... tools/testing/dart/dependency_graph.dart:73: // We emit events asynchronously so the graph can be build up in a small up in a small -> in small https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_p... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_p... tools/testing/dart/test_progress.dart:134: output.add('Command[${command.displayName}}]: $command'); extra } https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:109: builder.add(displayName); // FIXME(kustermann): Yes or No? fix this non-informative fixme :-) https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:112: for (var key in environmentOverrides.keys) builder.add(environmentOverrides[key]); Do we want to add the key to builder as well? https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:120: displayName != other.displayName || // FIXME(kustermann): Yes or No? same as above https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:129: add: if (environmentOverrides == null) return true; and remove null check below? https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:334: this.url, indentation https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:437: {String flavour: 'dartanalyzer'}) { I think we normally use US spelling (i.e., flavour -> flavor) https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:913: // FIXME(kustermann): This is wrong, we should give the expectations in in to command -> {in || to} command https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1262: Completer completer = new Completer(); rename this completer to terminationCompleter - to avoid confusion with _completer instance variable https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1447: * part of part of -> part of it? https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1470: // Make exactly *one* node in the dependency graph for every command. if this is read out of context it may be misunderstood (i.e. that even if two commands are doing the same we actually add it twice, which we don't https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1545: var newState; just set newState = dgraph.NodeState.Waiting here and eleminate else branch https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1637: int timeout = testCases.map((TestCase test) => test.timeout) this is sort of changing current "semantics" but I agree this is the way to do it. Please add a comment about this https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1685: * - [:maxProcesses < numberOfProcessesUsed:] isn't it the other way around? https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1695: // TODO(kustermann): The [timeout] parameter should be a property of Command agreed
LGTM https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1735: Future<CommandOutput> runCommand(node, Command command, int timeout) { you specify the type of the other variables, why not node? https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1735: Future<CommandOutput> runCommand(node, Command command, int timeout) { I don't understand our need to pass around both the node and command object. We already have the command as the userdata on the node right? We do this here and below
Try landing it soonish. https://codereview.chromium.org/21001003/diff/24001/tests/standalone/io/test_... File tests/standalone/io/test_runner_test.dart (right): https://codereview.chromium.org/21001003/diff/24001/tests/standalone/io/test_... tests/standalone/io/test_runner_test.dart:80: _makeCrashTestCase(name, expectations) { On 2013/08/01 18:23:52, ricow1 wrote: > where do call this from? 25 lines above. https://codereview.chromium.org/21001003/diff/24001/tests/utils/dependency_gr... File tests/utils/dependency_graph_test.dart (right): https://codereview.chromium.org/21001003/diff/24001/tests/utils/dependency_gr... tests/utils/dependency_graph_test.dart:59: for (var i=0; i<events.length; i++) { On 2013/08/01 18:23:52, ricow1 wrote: > could we indent this for loop 4 more spaces, this makes it hard to read Moved it one line up. https://codereview.chromium.org/21001003/diff/24001/tools/test.dart File tools/test.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/test.dart#newcode242 tools/test.dart:242: new ProcessQueue(firstConf, On 2013/08/01 18:23:52, ricow1 wrote: > it seems a little odd to pass in the first conf, but I understand the incentive > to do so. Could you add a small comment stating why we do that? This is actually not odd. Strictly speaking, our configuration maps are split into two groups - global settings - settings specific to a given "configuration" I added a comment. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/depend... File tools/testing/dart/dependency_graph.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/depend... tools/testing/dart/dependency_graph.dart:9: On 2013/08/01 18:23:52, ricow1 wrote: > Since this is general purpose it would be beneficial to add doc style comments > to this Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/depend... tools/testing/dart/dependency_graph.dart:73: // We emit events asynchronously so the graph can be build up in a small On 2013/08/01 18:23:52, ricow1 wrote: > up in a small -> in small Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_p... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_p... tools/testing/dart/test_progress.dart:134: output.add('Command[${command.displayName}}]: $command'); On 2013/08/01 18:23:52, ricow1 wrote: > extra } Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:109: builder.add(displayName); // FIXME(kustermann): Yes or No? On 2013/08/01 18:23:52, ricow1 wrote: > fix this non-informative fixme :-) Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:112: for (var key in environmentOverrides.keys) builder.add(environmentOverrides[key]); On 2013/08/01 18:23:52, ricow1 wrote: > Do we want to add the key to builder as well? I don't think we're getting many collisions anyway, but I've added the key as well. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:120: displayName != other.displayName || // FIXME(kustermann): Yes or No? On 2013/08/01 18:23:52, ricow1 wrote: > same as above Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:129: On 2013/08/01 18:23:52, ricow1 wrote: > add: > if (environmentOverrides == null) return true; > > and remove null check below? Again, this is not possible, Just because environmentOverrides == other.environmentOverrides == null we can't just return here, we've to compare the other fields as well. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:334: this.url, On 2013/08/01 18:23:52, ricow1 wrote: > indentation Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:437: {String flavour: 'dartanalyzer'}) { On 2013/08/01 18:23:52, ricow1 wrote: > I think we normally use US spelling (i.e., flavour -> flavor) Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:913: // FIXME(kustermann): This is wrong, we should give the expectations in On 2013/08/01 18:23:52, ricow1 wrote: > in to command -> {in || to} command Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1262: Completer completer = new Completer(); On 2013/08/01 18:23:52, ricow1 wrote: > rename this completer to terminationCompleter - to avoid confusion with > _completer instance variable Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1357: //print("STARTING $executable ${arguments.join(' ')} "); On 2013/08/01 13:26:21, ricow1 wrote: > commented out code Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1369: //print("STDOUT $line"); On 2013/08/01 13:26:21, ricow1 wrote: > commented out line Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1393: //print("STDERR: $line"); On 2013/08/01 13:26:21, ricow1 wrote: > commented out line Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1447: * part of On 2013/08/01 18:23:52, ricow1 wrote: > part of -> part of it? I think "to all TestCases that it is part of" is ok. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1470: // Make exactly *one* node in the dependency graph for every command. On 2013/08/01 18:23:52, ricow1 wrote: > if this is read out of context it may be misunderstood (i.e. that even if two > commands are doing the same we actually add it twice, which we don't Added more comments. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1545: var newState; On 2013/08/01 18:23:52, ricow1 wrote: > just set newState = dgraph.NodeState.Waiting here and eleminate else branch Done. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1637: int timeout = testCases.map((TestCase test) => test.timeout) On 2013/08/01 18:23:52, ricow1 wrote: > this is sort of changing current "semantics" but I agree this is the way to do > it. Please add a comment about this Just having TestCase.timeout is totally stupid, since we give the same timeout for all commands (tough they might have very different running times). Added comment. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1685: * - [:maxProcesses < numberOfProcessesUsed:] On 2013/08/01 18:23:52, ricow1 wrote: > isn't it the other way around? Yes, sorry. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1735: Future<CommandOutput> runCommand(node, Command command, int timeout) { On 2013/08/02 07:40:32, ricow1 wrote: > you specify the type of the other variables, why not node? This is a case where I would hit the 80 character limit -- 'dgraph.Node ' is just too long. I think it's not a big issue if it's not typed. I added the type to the 'CommandExecutor' interface. https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1735: Future<CommandOutput> runCommand(node, Command command, int timeout) { On 2013/08/02 07:40:32, ricow1 wrote: > I don't understand our need to pass around both the node and command object. We > already have the command as the userdata on the node right? > We do this here and below That is correct. Actually 'CommandeExecutor' should only be given a 'Command' and return a 'CommandOutput'. It shouldn't know about nodes. As you can see, we don't actually use 'node' here. But I added it to 'RecordingCommandExecutor' and 'ReplayingCommandExecutor' to ensure we've no dependencies (i.e. record/replay doesn't support dependencies [since the python script just executes one command after another one]). https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/utils.... File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/24001/tools/testing/dart/utils.... tools/testing/dart/utils.dart:104: if (data[i+j] != pattern[j]) { On 2013/08/01 13:26:21, ricow1 wrote: > space around + Done.
Message was sent while issue was closed.
Committed patchset #4 manually as r25760 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (left): https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1727: print('$i. $command'); Is there some sort of equivalent of this command now in the new version? We use this a lot to debug browser tests .
Message was sent while issue was closed.
https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (left): https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1727: print('$i. $command'); On 2013/08/06 23:42:36, Emily Fortuna wrote: > Is there some sort of equivalent of this command now in the new version? We use > this a lot to debug browser tests . Oops, please disregard that comment; I see the new output. I just wasn't expecting it to be as wordy.
Message was sent while issue was closed.
https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (left): https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:1727: print('$i. $command'); On 2013/08/06 23:44:53, Emily Fortuna wrote: > On 2013/08/06 23:42:36, Emily Fortuna wrote: > > Is there some sort of equivalent of this command now in the new version? We > use > > this a lot to debug browser tests . > > Oops, please disregard that comment; I see the new output. I just wasn't > expecting it to be as wordy. Can we also print the command for starting the http server? I see everything else in the new version but that.
Message was sent while issue was closed.
On 2013/08/06 23:46:37, Emily Fortuna wrote: > https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... > File tools/testing/dart/test_runner.dart (left): > > https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/test_r... > tools/testing/dart/test_runner.dart:1727: print('$i. $command'); > On 2013/08/06 23:44:53, Emily Fortuna wrote: > > On 2013/08/06 23:42:36, Emily Fortuna wrote: > > > Is there some sort of equivalent of this command now in the new version? We > > use > > > this a lot to debug browser tests . > > > > Oops, please disregard that comment; I see the new output. I just wasn't > > expecting it to be as wordy. > > Can we also print the command for starting the http server? I see everything > else in the new version but that. Sure. I committed: https://codereview.chromium.org//22532003 Actually I think using --verbose is now roughly as wordy as before. The order is just different (previously we printed out dart2js+browser command when we started running the first command of a TestCase). After this CL, different TestCases can share a dart2js command. So we only print out a Command if we actually run it.
Message was sent while issue was closed.
DBC in case anyone plans on copying this pattern elsewhere. https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/utils.... File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/utils.... tools/testing/dart/utils.dart:136: class HashCodeBuilder { This way of computing a hashCode creates a lot of polymorphism and other kinds of code that is hard to optimize away. So it is probably rather slow. I'd suggest a top-level function instead, for example: int extendHashCode(int hashCode, int other) { return ((((hashCode & 0x3FFFFFFF) * 31) & 0x3FFFFFFF) ^ (other & 0x3FFFFFFF)) & 0x3FFFFFFF; } Then use this function to compute hash codes like this: int computeHashCode() { int hashCode = extendHashCode(0, commandLine.hashCode); hashCode = extendHashCode(hashCode, displayName.hashCode) return hashCode; } Compared to using the builder: void _buildHashCode(HashCodeBuilder builder) { builder ..add(commandLine) ..add(displayName); } The advantage of the former is: * No object allocation in order to compute hashCodes. * No field access. * High probability that .hashCode invocations are mono-morphic. In addition, extendHashCode guarantees that all intermediate computations are within the 24-bit range (not just the result). This should also be faster.
Message was sent while issue was closed.
kustermann@google.com changed reviewers: - efortuna@google.com, ricow@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/utils.... File tools/testing/dart/utils.dart (right): https://codereview.chromium.org/21001003/diff/42001/tools/testing/dart/utils.... tools/testing/dart/utils.dart:136: class HashCodeBuilder { On 2015/05/18 15:48:12, ahe wrote: > This way of computing a hashCode creates a lot of polymorphism and other kinds > of code that is hard to optimize away. So it is probably rather slow. > > I'd suggest a top-level function instead, for example: > > int extendHashCode(int hashCode, int other) { > return ((((hashCode & 0x3FFFFFFF) * 31) & 0x3FFFFFFF) ^ (other & 0x3FFFFFFF)) > & 0x3FFFFFFF; > } > > Then use this function to compute hash codes like this: > > int computeHashCode() { > int hashCode = extendHashCode(0, commandLine.hashCode); > hashCode = extendHashCode(hashCode, displayName.hashCode) > return hashCode; > } > > Compared to using the builder: > > void _buildHashCode(HashCodeBuilder builder) { > builder > ..add(commandLine) > ..add(displayName); > } > > The advantage of the former is: > > * No object allocation in order to compute hashCodes. > > * No field access. > > * High probability that .hashCode invocations are mono-morphic. > > In addition, extendHashCode guarantees that all intermediate computations are > within the 24-bit range (not just the result). This should also be faster. Agreed, this is sub-optimal and could be optimized. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
