Add coverage tool to standalone tests

Add coverage tool to standalone tests

R=kustermann@google.com

Committed: https://code.google.com/p/dart/source/detail?r=23494

[ricow1, 2013-05-31 05:29:01]

DBC

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:47: Process.start(options.executable,
targetOpts).then((Process process) {
No need to use Process.start here, use Process.run instead, will simplify this a
lot

[kustermann, 2013-05-31 13:41:28]

lgtm with comments.

The main question I have is: Is this test testing what you need?

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:14: var coverageTargetUrl =
"tests/language/hello_dart_test.dart";
Using paths relative to dart/ requires this script to be always called from
within dart/. It's ok, but you could get the path to this script using 'new
Options().script' and navigate up.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:14: var coverageTargetUrl =
"tests/language/hello_dart_test.dart";
These are not really urls. You could rename them to 'coverageToolPath' or so.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:23: nextLineToMatch++;
If I see this correct, you actually don't verify the output of the coverage
tool. You only check that the source code of 'hello_dart_test.dart' is contained
in the output.
Is this what you want to do? If so, I'm fine with it.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:44: sourceLines = new
File(Uri.parse(coverageTargetUrl).path).readAsLinesSync();
Uri.path will give you always forward slashes AFAIK. You could use
'new Path(coverageTargetUrl).toNativePath()'.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:47: Process.start(options.executable,
targetOpts).then((Process process) {
On 2013/05/31 05:29:02, ricow1 wrote:
> No need to use Process.start here, use Process.run instead, will simplify this
a
> lot
agreed.

[hausner, 2013-05-31 21:53:02]

Thank you for the review and tips.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:14: var coverageTargetUrl =
"tests/language/hello_dart_test.dart";
On 2013/05/31 13:41:28, kustermann wrote:
> These are not really urls. You could rename them to 'coverageToolPath' or so.

Done.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:14: var coverageTargetUrl =
"tests/language/hello_dart_test.dart";
On 2013/05/31 13:41:28, kustermann wrote:
> Using paths relative to dart/ requires this script to be always called from
> within dart/. It's ok, but you could get the path to this script using 'new
> Options().script' and navigate up.

Changed to use paths relative to the path of this script. I could not find a way
to get to the absolute path of the root of the dart tree.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:23: nextLineToMatch++;
On 2013/05/31 13:41:28, kustermann wrote:
> If I see this correct, you actually don't verify the output of the coverage
> tool. You only check that the source code of 'hello_dart_test.dart' is
contained
> in the output.
> Is this what you want to do? If so, I'm fine with it.

Correct. I just want to make sure the coverage tool compiles and runs. I am not
interested in checking a specific coverage counter. By checking that the
coverage tool prints out the source it inspected, I assume the coverage tool ran
fine.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:44: sourceLines = new
File(Uri.parse(coverageTargetUrl).path).readAsLinesSync();
On 2013/05/31 13:41:28, kustermann wrote:
> Uri.path will give you always forward slashes AFAIK. You could use
> 'new Path(coverageTargetUrl).toNativePath()'.

Done.

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:47: Process.start(options.executable,
targetOpts).then((Process process) {
On 2013/05/31 05:29:02, ricow1 wrote:
> No need to use Process.start here, use Process.run instead, will simplify this
a
> lot

I don't know how I'd get to stdout and stderr of the child process if I use
run(). If you feel strongly i will do this in a later checkin.

[hausner, 2013-05-31 21:53:41]

Committed patchset #5 manually as r23494 (presubmit successful).

[ricow1, 2013-06-01 06:27:58]

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/15894015/diff/2001/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:47: Process.start(options.executable,
targetOpts).then((Process process) {
On 2013/05/31 21:53:02, hausner wrote:
> On 2013/05/31 05:29:02, ricow1 wrote:
> > No need to use Process.start here, use Process.run instead, will simplify
this
> a
> > lot
> 
> I don't know how I'd get to stdout and stderr of the child process if I use
> run(). If you feel strongly i will do this in a later checkin.
Process.run(bin, args).then((result) {
  print(result.stderr);
  print(result.stdout);
  print(result.exitCode);
  print(result.pid);
});