Issue 8835010: Support --special-command test option. As a special case support --valgrind option.

Issue 8835010: Support --special-command test option. As a special case support --valgrind option. (Closed)

Created:
9 years ago by Mads Ager (google)

Modified:
9 years ago

Reviewers:
Bill Hesse

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Support --special-command test option. As a special case support --valgrind option. R=whesse@google.com BUG= TEST= Committed: https://code.google.com/p/dart/source/detail?r=2174

Patch Set 2 : Fix minor issues. #

Total comments: 6

Patch Set 3 : Fix test runner test. #

Patch Set 4 : Address review comment. #

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+86 lines, -26 lines)			Patch
M	tests/standalone/src/TestRunnerTest.dart	View	1 2	2 chunks	+6 lines, -4 lines	0 comments	Download
M	tests/stub-generator/test_config.dart	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	tools/testing/dart/test_options.dart	View		4 chunks	+37 lines, -2 lines	0 comments	Download
M	tools/testing/dart/test_runner.dart	View	1 2 3	3 chunks	+36 lines, -12 lines	0 comments	Download
M	tools/testing/dart/test_suite.dart	View	1	4 chunks	+5 lines, -6 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Bill Hesse

LGTM. http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_options.dart File tools/testing/dart/test_options.dart (right): http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_options.dart#newcode42 tools/testing/dart/test_options.dart:42: command. Mention about no embedded spaces in arguments ...

9 years ago (2011-12-07 13:14:14 UTC) #2

Mads Ager (google)

9 years ago (2011-12-07 13:23:02 UTC) #3

http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_options...
File tools/testing/dart/test_options.dart (right):

http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_options...
tools/testing/dart/test_options.dart:42: command.
On 2011/12/07 13:14:14, Bill Hesse wrote:
> Mention about no embedded spaces in arguments here - no
> --flags="--foo --bar".

Do we have any flags at this point where that makes sense? I don't think we do
(other than now the special-commands flag itself).

http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_runner....
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:60: var newExcecutablePath = prefixSplit[0];
On 2011/12/07 13:14:14, Bill Hesse wrote:
> newExecutablePath.

Thanks. Done!

http://codereview.chromium.org/8835010/diff/5/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:69: var suffixSplit = prefix.split(' ');
On 2011/12/07 13:14:14, Bill Hesse wrote:
> This will not handle arguments with embedded spaces, such as
> --dart-flags="--foo --bar".  I don't think it is worth fixing, but maybe the
> issue could appear in the description of the flag?  Otherwise, someone will
have
> no idea why their stuff doesn't work.

At this point I don't think that can happen. All the string options that we have
are verified and cannot contain spaces in any case. The only exception is this
new flag. :-)

Expand Messages | Collapse Messages