Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11369216:
Added support for skipping redundant dart2js compilations. (Closed)
|
Created:
8 years, 1 month ago by kustermann Modified:
8 years ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdded support for skipping redundant dart2js compilations.
Currently, the buildbots run tests on every commit. But often neither
the tests nor the dart2js compiler did change (e.g. on status file updates).
The added code will check if the dependencies (i.e. files listed in
test.js.deps) or the dart2js snapshot did change. If not, the output
of the previous dart2js compilation is up to date and the compilation
step is skipped.
The skipping of redundant dart2js compilations works only if the '--use-sdk'
option was specified (because in this case the dart2js snapshot is used).
Committed: https://code.google.com/p/dart/source/detail?r=15080
Committed: https://code.google.com/p/dart/source/detail?r=16158
Patch Set 1 #
Total comments: 69
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : fixed long lines #
Total comments: 14
Patch Set 6 : #
Messages
Total messages: 14 (0 generated)
Please review this carefully!
First round of comments As discussed offline you should use the standard notation for testing files ending in: _test.dart https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... File tests/standalone/standalone.status (right): https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... tests/standalone/standalone.status:24: io/test_skipping_dart2js_compilation__helper: Skip You should add a comment here as to why we need to skip this, otherwise this is going to make somebody wonder and spend half an hour at some point https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:78: print('\n$_skippedCompilations dart2js compilations were skipped because ' I think you should make the text a little more generic by leaving out "dart2js" in the sentence - I assume that we will eventually be doing something similar for dart2dart. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:59: Dart2JsCommand(String this._jsOutFile, bool this._neverSkipCompilation, I know that not all of our code is actually strictly adhering to this - but I really like to either have all parameters on one line, or if that don't fit, one line per parameter (indented under the parameter from the main line) https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:66: List<Uri> readDepsFile(String path) { I think we should make this asynchronious https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:759: // NOTE: we need to have the same async + timeout handler behaviour as below long line https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:763: stdout.add("Skipped dart2js compilation because the old output is still up to date!"); long line https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:450: join(new Path('dart-sdk/lib/_internal/compiler/implementation/dart2js.dart.snapshot'))); Long line
Super excited to see that you are working on this! Did you forget to add the dependency file (test.js.deps)?
https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:44: void skippedCompilation() { _skippedCompilations++; } Maybe compilationSkipped is a better name (just for this function, not the counter or other functions)- it refers to an event that happened, rather than an object that might have state, behavior, or persistence. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:104: _printSkippedCompilationInfo(); In the long run, we may not want to print this out. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:764: if (processQueue != null) { We don't ever get to this point without a processQueue, do we? What runs the tests, if not a processQueue? https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:765: processQueue.logSkippedCompilation(); Other information gets to the progress indicator by being put in the testOutput class. Could we put this as a flag into testOutput, rather than tunneling through processQueue to the progress indicator? https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1515: Why not cache currentDirectoryPath here, the way testScriptPath is cached? https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1564: var cwd = new Path(new Directory.current().path); Can we cache this (new Path(new Directory.current().path))?
https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:104: _printSkippedCompilationInfo(); On 2012/11/14 09:37:34, Bill Hesse wrote: > In the long run, we may not want to print this out. I really think we should, at least have a flag that makes us do it (so, e.g. on the buildbot you can actually always see this).
I think this is a very good start. But I agree with Rico, it would probably be best if outputIsUpToDate returns a future. https://codereview.chromium.org/11369216/diff/1/tests/standalone/io/test_skip... File tests/standalone/io/test_skipping_dart2js_compilation__helper.dart (right): https://codereview.chromium.org/11369216/diff/1/tests/standalone/io/test_skip... tests/standalone/io/test_skipping_dart2js_compilation__helper.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file This file has an extra _ (underscore) in its name. https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... File tests/standalone/standalone.status (right): https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... tests/standalone/standalone.status:24: io/test_skipping_dart2js_compilation__helper: Skip This is not necessary. Only files ending with _test.dart are considered to be tests. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:79: 'the previous output was already up to date\n'); I would try to make this text shorter, for example: '\nSkipped compiling ${_skippedCompilations} up-to-date files.\n' https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:55: String _jsOutFile; We try to avoid abbreviating. So this should be _jsOutputFile. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:57: List<Uri> _bootstrapDeps; _bootstrapDependencies https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:59: Dart2JsCommand(String this._jsOutFile, bool this._neverSkipCompilation, Remove types from this.field parameters. They are spec'ed to be inferred from the corresponding fields. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:66: List<Uri> readDepsFile(String path) { Using the word "deps" file is fine in this case, because it actually reads a .deps file. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:71: var deps = new List<Uri>(); But these are dependencies :-) https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:81: var deps = readDepsFile("$_jsOutFile.deps"); dependencies https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:84: var jsOutTimestamp = TestUtils.timestampCache.getTimeStamp( jsOutput... https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:85: new Uri("file://$_jsOutFile")); How do you ensure this is a valid file URI? See: http://blogs.msdn.com/b/ie/archive/2006/12/06/file-uris-in-windows.aspx https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:88: var depTs = TestUtils.timestampCache.getTimeStamp(dep); In this case, abbreviation makes it really hard to read this code. So I suggest that you use dependencyTimestamp or lastModified. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:760: // otherwise we risk breaking code. Only one space after //. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:762: new Timer(0, (ignored) { You could avoid this if command.outputIsUpToDate returned a future. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:448: if (useDart2JsFromSdk) { Bail-out early: if (!useDart2jsFromSdk) return null; https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:450: join(new Path('dart-sdk/lib/_internal/compiler/implementation/dart2js.dart.snapshot'))); On 2012/11/14 08:53:28, ricow1 wrote: > Long line You can always split a string in two using: new Path('dart-sdk/lib/_internal/compiler/implementation/' 'dart2js.dart.snapshot') https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:451: return [new Uri("file://$snapshotPath")]; See http://blogs.msdn.com/b/ie/archive/2006/12/06/file-uris-in-windows.aspx https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:716: !useDart2JsFromSdk, dart2JsBootstrapDependencies, dartShellFileName, args)]; Long line. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:716: !useDart2JsFromSdk, dart2JsBootstrapDependencies, dartShellFileName, args)]; It is not necessary to pass in !useDart2JsFromSdk. It is implied by dart2JsBootstrapDependencies being null. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1483: Map<String,Date> _cache = <String,Date>{}; Add space after comma (twice). https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1486: * Returns the timestamp of the given [Uri] and caches the result for Try to write documentation comments so that the first paragraph is a summary/heading. For example: /** * Returns the (cached) timestamp of the given [uri]. * * If [uri] is a local file ... https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1493: Date getTimeStamp(Uri uri) { I would have called this "getLastModified".
PTAL. I've addressed your comments and rebased since the code now uses CommandOutput.compilationSkipped". https://codereview.chromium.org/11369216/diff/1/tests/standalone/io/test_skip... File tests/standalone/io/test_skipping_dart2js_compilation__helper.dart (right): https://codereview.chromium.org/11369216/diff/1/tests/standalone/io/test_skip... tests/standalone/io/test_skipping_dart2js_compilation__helper.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file On 2012/11/14 18:46:16, ahe wrote: > This file has an extra _ (underscore) in its name. Done. https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... File tests/standalone/standalone.status (right): https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... tests/standalone/standalone.status:24: io/test_skipping_dart2js_compilation__helper: Skip I'll remove this line. Since the helper dart script doesn't end with *_test.dart, it's not going to be executed anyway. On 2012/11/14 08:53:28, ricow1 wrote: > You should add a comment here as to why we need to skip this, otherwise this is > going to make somebody wonder and spend half an hour at some point https://codereview.chromium.org/11369216/diff/1/tests/standalone/standalone.s... tests/standalone/standalone.status:24: io/test_skipping_dart2js_compilation__helper: Skip On 2012/11/14 18:46:16, ahe wrote: > This is not necessary. Only files ending with _test.dart are considered to be > tests. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:44: void skippedCompilation() { _skippedCompilations++; } CommandOutput contains now a compilationSkipped field. On 2012/11/14 09:37:34, Bill Hesse wrote: > Maybe compilationSkipped is a better name (just for this function, not the > counter or other functions)- it refers to an event that happened, rather than an > object that might have state, behavior, or persistence. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:78: print('\n$_skippedCompilations dart2js compilations were skipped because ' On 2012/11/14 08:53:28, ricow1 wrote: > I think you should make the text a little more generic by leaving out "dart2js" > in the sentence - I assume that we will eventually be doing something similar > for dart2dart. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:79: 'the previous output was already up to date\n'); I think the current message is more descriptive. (Not the files are up-to-date but rather the compiled output) On 2012/11/14 18:46:16, ahe wrote: > I would try to make this text shorter, for example: > > '\nSkipped compiling ${_skippedCompilations} up-to-date files.\n' https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_progr... tools/testing/dart/test_progress.dart:104: _printSkippedCompilationInfo(); We could make it depend on '--report' but the ProgressIndicator doesn't get passed a configuration object. For now, let's just keep this. On 2012/11/14 09:47:32, ricow1 wrote: > On 2012/11/14 09:37:34, Bill Hesse wrote: > > In the long run, we may not want to print this out. > I really think we should, at least have a flag that makes us do it (so, e.g. on > the buildbot you can actually always see this). https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:55: String _jsOutFile; On 2012/11/14 18:46:16, ahe wrote: > We try to avoid abbreviating. So this should be _jsOutputFile. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:57: List<Uri> _bootstrapDeps; On 2012/11/14 18:46:16, ahe wrote: > _bootstrapDependencies Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:59: Dart2JsCommand(String this._jsOutFile, bool this._neverSkipCompilation, On 2012/11/14 08:53:28, ricow1 wrote: > I know that not all of our code is actually strictly adhering to this - but I > really like to either have all parameters on one line, or if that don't fit, one > line per parameter (indented under the parameter from the main line) Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:59: Dart2JsCommand(String this._jsOutFile, bool this._neverSkipCompilation, On 2012/11/14 18:46:16, ahe wrote: > Remove types from this.field parameters. They are spec'ed to be inferred from > the corresponding fields. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:66: List<Uri> readDepsFile(String path) { On 2012/11/14 08:53:28, ricow1 wrote: > I think we should make this asynchronious Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:71: var deps = new List<Uri>(); On 2012/11/14 18:46:16, ahe wrote: > But these are dependencies :-) Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:81: var deps = readDepsFile("$_jsOutFile.deps"); On 2012/11/14 18:46:16, ahe wrote: > dependencies Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:84: var jsOutTimestamp = TestUtils.timestampCache.getTimeStamp( On 2012/11/14 18:46:16, ahe wrote: > jsOutput... Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:85: new Uri("file://$_jsOutFile")); "StandardTestSuite.{makeCommands,_compileCommand}" call the Dart2JsCommand constructor. These two methods build the jsOutputFilename based on the output of "createOutputDirectory". "createOutputDirectory" in return, replaces '\\' by '/'. This is obviously bad code and should be fixed. But I don't think this CL should include these cleanups. On 2012/11/14 18:46:16, ahe wrote: > How do you ensure this is a valid file URI? > > See: http://blogs.msdn.com/b/ie/archive/2006/12/06/file-uris-in-windows.aspx https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:88: var depTs = TestUtils.timestampCache.getTimeStamp(dep); Done. But: Very often, longer names result in lines with more than 80 characters. Meaning we need to break the line into multiple lines. This in return often makes the code less readable than the original one. On 2012/11/14 18:46:16, ahe wrote: > In this case, abbreviation makes it really hard to read this code. So I suggest > that you use dependencyTimestamp or lastModified. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:759: // NOTE: we need to have the same async + timeout handler behaviour as below On 2012/11/14 08:53:28, ricow1 wrote: > long line Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:760: // otherwise we risk breaking code. On 2012/11/14 18:46:16, ahe wrote: > Only one space after //. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:762: new Timer(0, (ignored) { On 2012/11/14 18:46:16, ahe wrote: > You could avoid this if command.outputIsUpToDate returned a future. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:763: stdout.add("Skipped dart2js compilation because the old output is still up to date!"); On 2012/11/14 08:53:28, ricow1 wrote: > long line Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:764: if (processQueue != null) { Actually the test tests/standalone/io/test_runner_test.dart as well as the one included in this CL do run RunningProcess without a ProcessQueue. On 2012/11/14 09:37:34, Bill Hesse wrote: > We don't ever get to this point without a processQueue, do we? What runs the > tests, if not a processQueue? https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:765: processQueue.logSkippedCompilation(); On 2012/11/14 09:37:34, Bill Hesse wrote: > Other information gets to the progress indicator by being put in the testOutput > class. Could we put this as a flag into testOutput, rather than tunneling > through processQueue to the progress indicator? Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:448: if (useDart2JsFromSdk) { On 2012/11/14 18:46:16, ahe wrote: > Bail-out early: > > if (!useDart2jsFromSdk) return null; Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:450: join(new Path('dart-sdk/lib/_internal/compiler/implementation/dart2js.dart.snapshot'))); On 2012/11/14 08:53:28, ricow1 wrote: > Long line Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:450: join(new Path('dart-sdk/lib/_internal/compiler/implementation/dart2js.dart.snapshot'))); On 2012/11/14 18:46:16, ahe wrote: > On 2012/11/14 08:53:28, ricow1 wrote: > > Long line > > You can always split a string in two using: > > new Path('dart-sdk/lib/_internal/compiler/implementation/' > 'dart2js.dart.snapshot') Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:716: !useDart2JsFromSdk, dart2JsBootstrapDependencies, dartShellFileName, args)]; On 2012/11/14 18:46:16, ahe wrote: > Long line. Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:716: !useDart2JsFromSdk, dart2JsBootstrapDependencies, dartShellFileName, args)]; I changed dart2JsBootstrapDependencies to return [] instead of null in case the '--use-sdk' was not set. I think we should keep the neverSkipCompilation flag. On 2012/11/14 18:46:16, ahe wrote: > It is not necessary to pass in !useDart2JsFromSdk. It is implied by > dart2JsBootstrapDependencies being null. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1483: Map<String,Date> _cache = <String,Date>{}; On 2012/11/14 18:46:16, ahe wrote: > Add space after comma (twice). Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1486: * Returns the timestamp of the given [Uri] and caches the result for On 2012/11/14 18:46:16, ahe wrote: > Try to write documentation comments so that the first paragraph is a > summary/heading. For example: > > /** > * Returns the (cached) timestamp of the given [uri]. > * > * If [uri] is a local file ... Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1493: Date getTimeStamp(Uri uri) { On 2012/11/14 18:46:16, ahe wrote: > I would have called this "getLastModified". Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1515: On 2012/11/14 09:37:34, Bill Hesse wrote: > Why not cache currentDirectoryPath here, the way testScriptPath is cached? Done. https://codereview.chromium.org/11369216/diff/1/tools/testing/dart/test_suite... tools/testing/dart/test_suite.dart:1564: var cwd = new Path(new Directory.current().path); On 2012/11/14 09:37:34, Bill Hesse wrote: > Can we cache this (new Path(new Directory.current().path))? Done.
LGTM https://codereview.chromium.org/11369216/diff/10001/tests/standalone/io/skipp... File tests/standalone/io/skipping_dart2js_compilations_test.dart (right): https://codereview.chromium.org/11369216/diff/10001/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:147: var fs_noTestJs = new FsUtils(false, true, true, true); consider using named parameters for the booleans, this is hard to read https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_p... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_p... tools/testing/dart/test_progress.dart:81: 'the previous output was already up to date\n'); indent this under the other string https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:54: class Dart2JsCommand extends Command { We could generalize this to not be dart2js specific, e.g., CompilationCacheCommand - but I don't mind if we land as is for now https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:877: stdout.add("Skipped dart2js compilation because the old output is " Skipped dart2js compilation -> Skipped compilation (we will probably do this for dart2dart as well)
Fixed some issues with native vs. non-native paths. https://codereview.chromium.org/11369216/diff/10001/tests/standalone/io/skipp... File tests/standalone/io/skipping_dart2js_compilations_test.dart (right): https://codereview.chromium.org/11369216/diff/10001/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:147: var fs_noTestJs = new FsUtils(false, true, true, true); On 2012/11/19 08:19:17, ricow1 wrote: > consider using named parameters for the booleans, this is hard to read Done. https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_p... File tools/testing/dart/test_progress.dart (right): https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_p... tools/testing/dart/test_progress.dart:81: 'the previous output was already up to date\n'); On 2012/11/19 08:19:17, ricow1 wrote: > indent this under the other string Done. https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:54: class Dart2JsCommand extends Command { I agree with you, that we should generalize this. But the deps file is currently only emitted by the dart2js compiler. So for now, we should keep this. On 2012/11/19 08:19:17, ricow1 wrote: > We could generalize this to not be dart2js specific, e.g., > CompilationCacheCommand - but I don't mind if we land as is for now https://codereview.chromium.org/11369216/diff/10001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:877: stdout.add("Skipped dart2js compilation because the old output is " On 2012/11/19 08:19:17, ricow1 wrote: > Skipped dart2js compilation -> Skipped compilation > (we will probably do this for dart2dart as well) Done.
PTAL
https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:883: options.environment = new Map<String, String>.from(Platform.environment); Is this line too long? https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_s... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_s... tools/testing/dart/test_suite.dart:1615: static Path absolutePath(Path path) { We have many places in the code where we instead use new Path.fromNative(new File.fromPath(path).fullPathSync()). What is the distinction between these, and when should we use one rather than the other?
https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_r... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_r... tools/testing/dart/test_runner.dart:883: options.environment = new Map<String, String>.from(Platform.environment); On 2012/12/11 10:51:04, Bill Hesse wrote: > Is this line too long? Done. https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_s... File tools/testing/dart/test_suite.dart (right): https://codereview.chromium.org/11369216/diff/19001/tools/testing/dart/test_s... tools/testing/dart/test_suite.dart:1615: static Path absolutePath(Path path) { On 2012/12/11 10:51:04, Bill Hesse wrote: > We have many places in the code where we instead use > new Path.fromNative(new File.fromPath(path).fullPathSync()). > What is the distinction between these, and when should we use one rather than > the other? Writing "new Path.fromNative(new File.fromPath(path).fullPathSync())" each time when we need an absolute path is just horrible. Secondly, we may want to get an absolute path from a relative path of a directory. So it doesn't make sense to use "new File()" if we have a relative directory (and AFAIK the "Directory" class doesn't have a "fullPathSync()" method)! IMHO, converting relative paths to absolute paths should be done by the Path class. It just doesn't belong to the File/Directory classes. (This would also make the "Path->File->String->Path conversions obsolete). (In Python for example, the "os.path" module contains a "abspath" function [http://docs.python.org/2/library/os.path.html]) I know that the Path class is currently "stupid", but maybe we should make it more intelligent?
LGTM https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... File tests/standalone/io/skipping_dart2js_compilations_helper.dart (right): https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_helper.dart:5: #import ('dart:io'); #import -> import. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... File tests/standalone/io/skipping_dart2js_compilations_test.dart (right): https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:4: Please add a comment explaining the purpose of this test. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:5: #import("dart:io"); #import -> import. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:8: #import("../../../tools/testing/dart/test_suite.dart"); It would be nice if these were imported with prefixes. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:13: class FsUtils { What does this class do? https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:24: tempDir = new Directory('').createTempSync(); I'm allergic to doing work in constructors. I think it is poor coding style. I go straight into anaphylactic shock when you start creating files in a constructor. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:132: TestCase MakeTestCase(String testName, TestCompletedHandler completedHandler) { MakeTestCase -> makeTestCase.
https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... File tests/standalone/io/skipping_dart2js_compilations_helper.dart (right): https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_helper.dart:5: #import ('dart:io'); On 2012/12/11 14:34:45, ahe wrote: > #import -> import. Done. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... File tests/standalone/io/skipping_dart2js_compilations_test.dart (right): https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:4: On 2012/12/11 14:34:45, ahe wrote: > Please add a comment explaining the purpose of this test. Done. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:5: #import("dart:io"); On 2012/12/11 14:34:45, ahe wrote: > #import -> import. Done. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:8: #import("../../../tools/testing/dart/test_suite.dart"); On 2012/12/11 14:34:45, ahe wrote: > It would be nice if these were imported with prefixes. Done. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:13: class FsUtils { On 2012/12/11 14:34:45, ahe wrote: > What does this class do? I renamed it now to FileUtils. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:24: tempDir = new Directory('').createTempSync(); On 2012/12/11 14:34:45, ahe wrote: > I'm allergic to doing work in constructors. I think it is poor coding style. I > go straight into anaphylactic shock when you start creating files in a > constructor. IMHO neither readability, nor code size would be improved by making it an extra method. And this way, we're sure that after the object had been created, all the necessary files have been created as well. But I agree with you in general: normally we don't want constructors to do a lot of work. https://codereview.chromium.org/11369216/diff/23003/tests/standalone/io/skipp... tests/standalone/io/skipping_dart2js_compilations_test.dart:132: TestCase MakeTestCase(String testName, TestCompletedHandler completedHandler) { On 2012/12/11 14:34:45, ahe wrote: > MakeTestCase -> makeTestCase. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
