Support for pixel layout tests.

Support for pixel layout tests.
Committed: https://code.google.com/p/dart/source/detail?r=12655

[Siggi Cherem (dart-lang), 2012-09-14 20:01:23]

just some general high-level comments so far

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
utils/testrunner/dart_wrap_task.dart:27: // wrapped Dart program that in turn
invokes DRT to run the
we've talked about this before, but I think we should revisit the question of
whether we want to have this feature of running test suites as layout tests - it
adds a lot of complexity in the test runner (compared to just treating a layout
test as a stand-alone program) and I'm not so sure how much people will find it
useful.

Regardless of what we decide, I also think we should support the other scenario,
that is, letting people write a layout test without using unittest all together
(run this program in the browser, return what it looks like).

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
utils/testrunner/dart_wrap_task.dart:31: var childFile =
'${destFile.substring(0, destFile.length-5)}-child.dart';
style: please add spaces around operators (destFile.length - 5).

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
utils/testrunner/dart_wrap_task.dart:36: sourceName.substring(0,
sourceName.length-5)); // Strip .dart.
ditto

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
utils/testrunner/dart_wrap_task.dart:191: sbuf.add("""
could we move a lot of this 'generated code' into a runtime library?

Basically, generate little code and call into a library of the testrunner
runtime?

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/html...
File utils/testrunner/html_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/html...
utils/testrunner/html_wrap_task.dart:59:
${(config.layoutText||config.layoutPixel)?'':
spaces around all operators: || ? :

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/test...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/test...
utils/testrunner/testrunner.dart:57: * test file (withouty extension) and with
the test name as the file name.
withouty -> without

[gram, 2012-09-17 20:33:39]

Okay, I have a significant refactoring done now; should be much better.


On 2012/09/14 20:01:23, sigmund wrote:
> just some general high-level comments so far
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
> File utils/testrunner/dart_wrap_task.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
> utils/testrunner/dart_wrap_task.dart:27: // wrapped Dart program that in turn
> invokes DRT to run the
> we've talked about this before, but I think we should revisit the question of
> whether we want to have this feature of running test suites as layout tests -
it
> adds a lot of complexity in the test runner (compared to just treating a
layout
> test as a stand-alone program) and I'm not so sure how much people will find
it
> useful.
> 
> Regardless of what we decide, I also think we should support the other
scenario,
> that is, letting people write a layout test without using unittest all
together
> (run this program in the browser, return what it looks like).
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
> utils/testrunner/dart_wrap_task.dart:31: var childFile =
> '${destFile.substring(0, destFile.length-5)}-child.dart';
> style: please add spaces around operators (destFile.length - 5).
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
> utils/testrunner/dart_wrap_task.dart:36: sourceName.substring(0,
> sourceName.length-5)); // Strip .dart.
> ditto
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
> utils/testrunner/dart_wrap_task.dart:191: sbuf.add("""
> could we move a lot of this 'generated code' into a runtime library?
> 
> Basically, generate little code and call into a library of the testrunner
> runtime?
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/html...
> File utils/testrunner/html_wrap_task.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/html...
> utils/testrunner/html_wrap_task.dart:59:
> ${(config.layoutText||config.layoutPixel)?'':
> spaces around all operators: || ? :
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/test...
> File utils/testrunner/testrunner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/test...
> utils/testrunner/testrunner.dart:57: * test file (withouty extension) and with
> the test name as the file name.
> withouty -> without

[gram, 2012-09-19 23:24:38]

PTAL, it is working now. I removed the HTTP server stuff and will add that back
in the next round.

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
utils/testrunner/dart_wrap_task.dart:31: var childFile =
'${destFile.substring(0, destFile.length-5)}-child.dart';
On 2012/09/14 20:01:23, sigmund wrote:
> style: please add spaces around operators (destFile.length - 5).

Done.

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/dart...
utils/testrunner/dart_wrap_task.dart:36: sourceName.substring(0,
sourceName.length-5)); // Strip .dart.
On 2012/09/14 20:01:23, sigmund wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/html...
File utils/testrunner/html_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/11/utils/testrunner/html...
utils/testrunner/html_wrap_task.dart:59:
${(config.layoutText||config.layoutPixel)?'':
On 2012/09/14 20:01:23, sigmund wrote:
> spaces around all operators: || ? :

Done.

[Siggi Cherem (dart-lang), 2012-09-20 17:37:27]

moving the runtime file out makes things so much better. Thanks!

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:71: if (config.runInBrowser) {
it would be nice to pull out this and the other 'if's further below, so you can
put the entire code in a single string. E.g.

if (config.runInBrowser) {
  extraImports = "#import('dart:html')";
  notifyDoneBody = "window.postMessage('done', '*')";
  tprintBody = "query....";
} else {
  ... // the other defs
}
if (...) {
  action = "...";
}

createFile(destFile, """
#library('layout_test');
$extraImport;
...
main() {
 ...
 sumarize = ...
 notifyDone = (e) => $notifyDoneBody;
 tprint = () => $tprintBody;
 action = $action;
 runTests(test.main);
}
"""

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:128: sbuf.add("""
seems like we can get rid of the buffer and return the string directly :)

layoutTestWrapper(String sourceName, String libDirectory) => """
...
""";

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:175: return sbuf.toString();
ditto

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/ht...
File utils/testrunner/html_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/ht...
utils/testrunner/html_wrap_task.dart:59: ${(config.layoutText ||
config.layoutPixel)?'':
nit: spaces around ? and :
also, like in my other comment, this might read more easily if we put the
condition upfront, store the actual line in a string variable that is injected
here.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
File utils/testrunner/layout_test_controller.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:1: // The following set of
variables should be set by the caller.
several comments here:
- add copyright notice header
- add #library?
- add general library comment (e.g. this is a runtime library used by code
generated by 'testrunner' to run layout tests)
- clarify "set by the caller": how? when? the caller of what?

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:44: [ testname = '', testTime = '',
result = '',
nit: no extra space after [ and before ]

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:111: exit(failCount > -0 ? -1 : 0);
-0 ?

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:118: var process =
Process.start(drt, [ url ]);
nit: [ url ] => [url] (no spaces)

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:125: line =
stdoutStringStream.readLine();
I feel like this logic occurs a lot, is there any library code (or can we create
a common utility method) that takes a stream and returns a Future<List<String>>?

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:163: if (!expected.existsSync()) {
combine with prev line (flatten):
} else if (!expected.existsSync()) {
 ...
} else {

}

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:168: fail(start, label,
'Expectation file has wrong length');
it's still useful to show some diffs in this case (e.g. some times the
difference is an empty line in the middle).

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:198: var process =
Process.start(drt, [ "$url'-p" ]);
nit: [ "$url'-p" ] => ["$url'-p"] (no spaces)

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:238: pos = idx+1;
nit: spaces around +

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
File utils/testrunner/layout_test_runner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_runner.dart:1: /** The filters must be set by the
caller. */
+ copyright
+ library

since this comment is not a dartdoc associated with the following field, you
might want to switch it to a normal comment (e.g. //)

also, same question as in the other file, who is the caller?

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_runner.dart:32: // Create the set of test cases.
nit: always add an empty line before a line comment (here and below)

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_runner.dart:50: 
nit: remove empty line

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/no...
File utils/testrunner/non_layout_test_runner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/no...
utils/testrunner/non_layout_test_runner.dart:1: /** Whether to include elapsed
time. */
+ copyright, + library, + general doc for the library

also, it would be nice to have a real name for this, e.g instead of
non_layout_test_runner, can we call it unit_test_runner? logical_test_runner?
normal_test_runner, traditional_test_runner?

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/no...
utils/testrunner/non_layout_test_runner.dart:243: // Main
?

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/no...
utils/testrunner/non_layout_test_runner.dart:275: 
remove line

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/te...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/te...
utils/testrunner/testrunner.dart:57: * test file (withouty extension) and with
the test name as the file name.
withouty => without

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/ut...
File utils/testrunner/utils.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:114: String getRunnerDirectory() {
should this be a getter instead?

[gram, 2012-09-20 18:58:03]

ptal

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:71: if (config.runInBrowser) {
On 2012/09/20 17:37:27, sigmund wrote:
> it would be nice to pull out this and the other 'if's further below, so you
can
> put the entire code in a single string. E.g.
> 
> if (config.runInBrowser) {
>   extraImports = "#import('dart:html')";
>   notifyDoneBody = "window.postMessage('done', '*')";
>   tprintBody = "query....";
> } else {
>   ... // the other defs
> }
> if (...) {
>   action = "...";
> }
> 
> createFile(destFile, """
> #library('layout_test');
> $extraImport;
> ...
> main() {
>  ...
>  sumarize = ...
>  notifyDone = (e) => $notifyDoneBody;
>  tprint = () => $tprintBody;
>  action = $action;
>  runTests(test.main);
> }
> """
>  

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:128: sbuf.add("""
On 2012/09/20 17:37:27, sigmund wrote:
> seems like we can get rid of the buffer and return the string directly :)
> 
> layoutTestWrapper(String sourceName, String libDirectory) => """
> ...
> """;

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:175: return sbuf.toString();
On 2012/09/20 17:37:27, sigmund wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/ht...
File utils/testrunner/html_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/ht...
utils/testrunner/html_wrap_task.dart:59: ${(config.layoutText ||
config.layoutPixel)?'':
On 2012/09/20 17:37:27, sigmund wrote:
> nit: spaces around ? and :
> also, like in my other comment, this might read more easily if we put the
> condition upfront, store the actual line in a string variable that is injected
> here.

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
File utils/testrunner/layout_test_controller.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:1: // The following set of
variables should be set by the caller.
On 2012/09/20 17:37:27, sigmund wrote:
> several comments here:
> - add copyright notice header
> - add #library?
> - add general library comment (e.g. this is a runtime library used by code
> generated by 'testrunner' to run layout tests)
> - clarify "set by the caller": how? when? the caller of what?

This is #sourced by the generated Dart file, hence no #directives.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:44: [ testname = '', testTime = '',
result = '',
On 2012/09/20 17:37:27, sigmund wrote:
> nit: no extra space after [ and before ]

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:111: exit(failCount > -0 ? -1 : 0);
On 2012/09/20 17:37:27, sigmund wrote:
> -0 ?

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:118: var process =
Process.start(drt, [ url ]);
On 2012/09/20 17:37:27, sigmund wrote:
> nit: [ url ] => [url] (no spaces)

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:125: line =
stdoutStringStream.readLine();
On 2012/09/20 17:37:27, sigmund wrote:
> I feel like this logic occurs a lot, is there any library code (or can we
create
> a common utility method) that takes a stream and returns a
Future<List<String>>?

It would be nice if the IO library had that; I don't think it belongs here
though.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:163: if (!expected.existsSync()) {
On 2012/09/20 17:37:27, sigmund wrote:
> combine with prev line (flatten):
> } else if (!expected.existsSync()) {
>  ...
> } else {
> 
> }

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:168: fail(start, label,
'Expectation file has wrong length');
On 2012/09/20 17:37:27, sigmund wrote:
> it's still useful to show some diffs in this case (e.g. some times the
> difference is an empty line in the middle). 

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:198: var process =
Process.start(drt, [ "$url'-p" ]);
On 2012/09/20 17:37:27, sigmund wrote:
> nit: [ "$url'-p" ] => ["$url'-p"] (no spaces)

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:238: pos = idx+1;
On 2012/09/20 17:37:27, sigmund wrote:
> nit: spaces around + 

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
File utils/testrunner/layout_test_runner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_runner.dart:1: /** The filters must be set by the
caller. */
On 2012/09/20 17:37:27, sigmund wrote:
> + copyright
> + library
> 
> since this comment is not a dartdoc associated with the following field, you
> might want to switch it to a normal comment (e.g. //)
> 
> also, same question as in the other file, who is the caller?

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_runner.dart:32: // Create the set of test cases.
On 2012/09/20 17:37:27, sigmund wrote:
> nit: always add an empty line before a line comment (here and below)

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_runner.dart:50: 
On 2012/09/20 17:37:27, sigmund wrote:
> nit: remove empty line

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/no...
File utils/testrunner/non_layout_test_runner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/no...
utils/testrunner/non_layout_test_runner.dart:243: // Main
On 2012/09/20 17:37:27, sigmund wrote:
> ?

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/te...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/te...
utils/testrunner/testrunner.dart:57: * test file (withouty extension) and with
the test name as the file name.
On 2012/09/20 17:37:27, sigmund wrote:
> withouty => without

Done.

[Siggi Cherem (dart-lang), 2012-09-20 19:56:27]

lgtm!

Pulling the conditionals before building the ouput made the code a lot easier to
read, thanks!

Just a couple minor comments below.

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
File utils/testrunner/layout_test_controller.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8001/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:125: line =
stdoutStringStream.readLine();
On 2012/09/20 18:58:03, gram wrote:
> On 2012/09/20 17:37:27, sigmund wrote:
> > I feel like this logic occurs a lot, is there any library code (or can we
> create
> > a common utility method) that takes a stream and returns a
> Future<List<String>>?
> 
> It would be nice if the IO library had that; I don't think it belongs here
> though.
> 

We could put it in utils.dart for now?

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:25: var libDirectory =  runnerDirectory;
nit: seems you don't need the local variable anymore, there is only one use on
each branch. OW, remove extra space (there seem to be 2)

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:122: """
nit: I prefer putting this on the previous line.

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
File utils/testrunner/layout_test_controller.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:175: if (lines[j] != stdout[i+j]) {
nit: spaces around + (here and next line)

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:264: for (var j = 0; j < len; j++)
{
seems like this loop and the one in the other onExit are very similar, can they
be pull out into a reusable function? (bool compareLists(List a, List b)?)

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:265: if (bytes[j] != stdout[pos+j])
{
ditto and next line

[gram, 2012-09-20 20:08:17]

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:25: var libDirectory =  runnerDirectory;
On 2012/09/20 19:56:27, sigmund wrote:
> nit: seems you don't need the local variable anymore, there is only one use on
> each branch. OW, remove extra space (there seem to be 2)

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:122: """
On 2012/09/20 19:56:27, sigmund wrote:
> nit: I prefer putting this on the previous line.

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
File utils/testrunner/layout_test_controller.dart (right):

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:175: if (lines[j] != stdout[i+j]) {
On 2012/09/20 19:56:27, sigmund wrote:
> nit: spaces around + (here and next line)

Done.

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:264: for (var j = 0; j < len; j++)
{
On 2012/09/20 19:56:27, sigmund wrote:
> seems like this loop and the one in the other onExit are very similar, can
they
> be pull out into a reusable function? (bool compareLists(List a, List b)?)


We're comparing items at different offsets, so a plain compareLists wouldn't
work; we'd need to pass base offsets or make a copy of the list that is not
zero-based. I don't think it is really worth it.

https://chromiumcodereview.appspot.com/10909240/diff/8005/utils/testrunner/la...
utils/testrunner/layout_test_controller.dart:265: if (bytes[j] != stdout[pos+j])
{
On 2012/09/20 19:56:27, sigmund wrote:
> ditto and next line

Done.