Changes to how we handle fancy stacks:

Changes to how we handle fancy stacks:

- in the base config they are disabled
- in htmnl config they are enabled unless a certain meta tag is present in the
  page.
- in vm config they are enabled unless a --no-fancy-stacks flag is in the
  Options.

The test runner adds a fancy_stacks flag that can be used to control this
behavior, but by default turns off fancy stacks for all tests.

There were a couple of test runner tests that were affected and have kludgy
fixes where I strip out the new argument from Options first.

This has passed a baseline set of tests; I'm still running a more comprehensive
set.

R=kustermann@google.com, nweiz@google.com

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

[gram, 2013-08-14 23:40:48]

Peter should also weigh in on whether we should disabling both filtering and
reformatting, or just filtering.

[nweiz, 2013-08-14 23:56:59]

I'm still in favor of having fancy-but-unfiltered traces be the default for
test.dart.

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/src/test_cas...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/src/test_cas...
pkg/unittest/lib/src/test_case.dart:46: get stackTrace => _stackTrace;
I don't like making this dynamically-typed. This will make any code that
accesses it type check correctly but fail at runtime in one or the other
configuration.

It would be better to leave this typed as Trace, and then have a separate
stackTraceString field that's always a string representation of a stack trace,
regardless of whether fancy traces are enabled.

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/unittest.dart
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:862: */
This method is now typed incorrectly. See my comments on the [stackTrace]
variable; this should similarly continue to return a Trace, and just return null
if formatStacks is not set.

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/vm_config.dart
File pkg/unittest/lib/vm_config.dart (right):

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/vm_config.da...
pkg/unittest/lib/vm_config.dart:43: formatStacks = !(new
Options().arguments.contains('--no-fancy-stacks'));
Add a TODO to use the args package for more robust argument handling.

Style nit: the parens here are unnecessary.

https://codereview.chromium.org/22859009/diff/1/tools/testing/dart/test_optio...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/22859009/diff/1/tools/testing/dart/test_optio...
tools/testing/dart/test_options.dart:327: ['--fancy_stacks'],
This option should be hyphenated rather than underscored.

[gram, 2013-08-15 22:36:43]

PTAL.

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/src/test_cas...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/src/test_cas...
pkg/unittest/lib/src/test_case.dart:46: get stackTrace => _stackTrace;
On 2013/08/14 23:56:59, nweiz wrote:
> I don't like making this dynamically-typed. This will make any code that
> accesses it type check correctly but fail at runtime in one or the other
> configuration.
> 
> It would be better to leave this typed as Trace, and then have a separate
> stackTraceString field that's always a string representation of a stack trace,
> regardless of whether fancy traces are enabled.

Done.

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/unittest.dart
File pkg/unittest/lib/unittest.dart (right):

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/unittest.dar...
pkg/unittest/lib/unittest.dart:862: */
On 2013/08/14 23:56:59, nweiz wrote:
> This method is now typed incorrectly. See my comments on the [stackTrace]
> variable; this should similarly continue to return a Trace, and just return
null
> if formatStacks is not set.

Done.

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/vm_config.dart
File pkg/unittest/lib/vm_config.dart (right):

https://codereview.chromium.org/22859009/diff/1/pkg/unittest/lib/vm_config.da...
pkg/unittest/lib/vm_config.dart:43: formatStacks = !(new
Options().arguments.contains('--no-fancy-stacks'));
On 2013/08/14 23:56:59, nweiz wrote:
> Add a TODO to use the args package for more robust argument handling.
> 
> Style nit: the parens here are unnecessary.

Done.

https://codereview.chromium.org/22859009/diff/1/tools/testing/dart/test_optio...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/22859009/diff/1/tools/testing/dart/test_optio...
tools/testing/dart/test_options.dart:327: ['--fancy_stacks'],
On 2013/08/14 23:56:59, nweiz wrote:
> This option should be hyphenated rather than underscored.

I saw others here that had underscores and thought I was following a convention
(albeit a broken one). Looking more carefully now I see that the options are
riddled with inconsistencies. E.g. --local_ip, --record_to_file,
--replay_from_file, --use_browser_controller, but --vm-options,
--write-debug-log, etc.

Anyway, while I'm unhappy to see we are so inconsistent, I'm glad that that
allows me to change this to hyphens :-)

[nweiz, 2013-08-15 23:03:25]

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:58: final _testLogBuffer = <Pair<String,
String>>[];
Trace implements StackTrace, so you can make this a Pair<String, StackTrace> and
avoid converting to a string until you actually need a string representation.

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:160: _testLogBuffer.add(new Pair<String,
String>(reason, trace.toString()));
Why are you eagerly converting the stack trace to a string rather than just
making it a Pair<String, StackTrace>?

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/test_...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/test_...
pkg/unittest/lib/src/test_case.dart:46: get stackTrace => _stackTrace;
The getter should also be typed.

Now there's no way to get the parsed stack trace. There should be a getter for
that as well, with documentation that it may be null.

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/test_...
pkg/unittest/lib/src/test_case.dart:133: void _setResult(String testResult,
String messageText, stack) {
[stack] here should be typed as StackTrace.

[gram, 2013-08-16 16:56:45]

PTAL

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
File pkg/unittest/lib/src/config.dart (right):

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:58: final _testLogBuffer = <Pair<String,
String>>[];
On 2013/08/15 23:03:25, nweiz wrote:
> Trace implements StackTrace, so you can make this a Pair<String, StackTrace>
and
> avoid converting to a string until you actually need a string representation.

Done.

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:58: final _testLogBuffer = <Pair<String,
String>>[];
On 2013/08/15 23:03:25, nweiz wrote:
> Trace implements StackTrace, so you can make this a Pair<String, StackTrace>
and
> avoid converting to a string until you actually need a string representation.

Done.

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/confi...
pkg/unittest/lib/src/config.dart:160: _testLogBuffer.add(new Pair<String,
String>(reason, trace.toString()));
On 2013/08/15 23:03:25, nweiz wrote:
> Why are you eagerly converting the stack trace to a string rather than just
> making it a Pair<String, StackTrace>?

Because I didn't realize Trace <- StackTrace. :-)

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/test_...
File pkg/unittest/lib/src/test_case.dart (right):

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/test_...
pkg/unittest/lib/src/test_case.dart:46: get stackTrace => _stackTrace;
On 2013/08/15 23:03:25, nweiz wrote:
> The getter should also be typed.
> 
> Now there's no way to get the parsed stack trace. There should be a getter for
> that as well, with documentation that it may be null.

I'm changing it to StackTrace; I wasn't aware that Trace was a subclass of
StackTrace. I don't think we need two getters in this case.

https://codereview.chromium.org/22859009/diff/8001/pkg/unittest/lib/src/test_...
pkg/unittest/lib/src/test_case.dart:133: void _setResult(String testResult,
String messageText, stack) {
On 2013/08/15 23:03:25, nweiz wrote:
> [stack] here should be typed as StackTrace.

Done.

[nweiz, 2013-08-16 19:09:39]

lgtm

[gram, 2013-08-21 00:18:59]

PTAL; I have made a few changes. In particular, test.dart now supports two
separate flags: --raw-stacks and --filter-stacks, both defaulting to false; the
net effect is that stack traces are reformatted but frames are not filtered.

[nweiz, 2013-08-21 01:03:53]

lgtm

[ahe, 2013-08-21 11:36:57]

Martin + Rico, could one of you take a look at these changes to the test runner.

Graham is ensuring that stack trace parsing is turned off when we run test.py.
But for users that don't use test.py, stack trace parsing is turned on by
default.

In order to achieve this, options must be passed to the test. The only way I
know to do so is to add additional HTML tags to the html page used to run the
test.

[ricow1, 2013-08-21 12:44:10]

https://codereview.chromium.org/22859009/diff/31001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/22859009/diff/31001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:798: if (configuration['raw-stacks']) {
does this work, adding flags _after_ the dart file? (the dart file is above).
Normally these flags are actually given to the executing dart program

[ahe, 2013-08-21 12:48:21]

https://codereview.chromium.org/22859009/diff/31001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/22859009/diff/31001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:798: if (configuration['raw-stacks']) {
On 2013/08/21 12:44:10, ricow1 wrote:
> does this work, adding flags _after_ the dart file? (the dart file is above).
> Normally these flags are actually given to the executing dart program

This is a flag to the Dart program, not the Dart VM.

I'd be concerned about passing command line arguments to Dart programs. It makes
it impossible to test new Options().arguments.

My understanding was that we agreed that the default in non-html configs would
be to not parse stack traces.

[gram, 2013-08-21 17:12:38]

https://codereview.chromium.org/22859009/diff/31001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/22859009/diff/31001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:798: if (configuration['raw-stacks']) {
On 2013/08/21 12:48:21, ahe wrote:
> On 2013/08/21 12:44:10, ricow1 wrote:
> > does this work, adding flags _after_ the dart file? (the dart file is
above).
> > Normally these flags are actually given to the executing dart program
> 
> This is a flag to the Dart program, not the Dart VM.
> 
> I'd be concerned about passing command line arguments to Dart programs. It
makes
> it impossible to test new Options().arguments.
> 
> My understanding was that we agreed that the default in non-html configs would
> be to not parse stack traces.

Yes, these are passed to the Dart test. As it provides flexibility in handling
stack traces, it seemed worth doing. It doesn't make it impossible to test
Options (the options test was modified to deal with this); it just makes the
Options tests a bit more tricky to diagnose if they fail, but in that case it is
easy to get these arguments to be excluded by passing the --filter-stacks option
to test.dart. If you look at the Options tests I think you will agree that this
change is incredibly unlikely to introduce a false positive or negative, and the
test should still catch failures if Options actually breaks.

[gram, 2013-08-22 17:56:05]

(Sorry for all the apparent patches; every time I try to do a gcl upload I get
an error saying try again, so I did. The error never went away but the files do
seem to have been uploaded - and uploaded - and...)

Trying this one more time. Hopefully this version will be satisfactory to
everyone.

- I have removed all command line flags. This means Options tests are
unaffected, and the changes to the test harness are isolated to just adding the
meta header to the HTML wrappers
- the default config now turns off stack frame filtering
- the html config will turn filtering back on unless it is suppressed by the
meta header
- the vm config will turn filtering back on
- fancy stack formatting happens regardless; it is just the filtering of stack
frames that is affected by this CL. So internally we will get full stacks,
nicely formatted. Externally we will in most cases get filtered and formatted
stacks (I'll have to make an update to the unit test article clarifying the
behavior of the default config).

[kustermann, 2013-08-22 21:40:52]

LGTM

https://codereview.chromium.org/22859009/diff/60007/tools/testing/dart/browse...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/22859009/diff/60007/tools/testing/dart/browse...
tools/testing/dart/browser_test.dart:47: <meta name="dart.unittest"
content="full-stack-traces">
You removed accidentally </head>!

[gram, 2013-08-22 21:47:57]

Committed patchset #15 manually as r26560 (presubmit successful).

[kevmoo-old, 2013-08-22 22:30:58]

Looks like you checked lib/src/config.dart back in after my rename?

[gram, 2013-08-22 22:32:41]

On 2013/08/22 22:30:58, kevmoo wrote:
> Looks like you checked lib/src/config.dart back in after my rename?

Ah, the joys of svn.