Configurable package folding #650

grouma · 2017-06-16T22:15:49Z

Allow StackTraceMapper to be serialized and deserialized
Send StackTraceMapper to remote isolate
Move all test chaining to the remote isolate
Enable new configurations for stack trace folding
Add corresponding tests

This will close #416

nex3

This also needs tests in configuration_test.dart.

nex3 · 2017-06-16T22:47:39Z

CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.12.23
+
+* Add a `fold_stack_frame` field for `dart_test.yaml`. This will


nex3 · 2017-06-16T22:48:13Z

doc/configuration.md

+
+This field indicates which packages will be folded when producing stack traces.
+Packages contained in the `exclude` option will be folded. If `only` is provided,
+all packages not contained in this list will be folded. If no options are provided,


nex3 · 2017-06-16T22:49:41Z

doc/configuration.md


+### `fold_stack_frames`
+
+This field indicates which packages will be folded when producing stack traces.


I don't think it'll be clear to an unfamiliar user what "folding" means in this context. Maybe "This field controls which packages' stack frames will be folded away when displaying stack traces." It would also be good to include an example stack trace.

nex3 · 2017-06-16T22:51:35Z

doc/configuration.md

+This field indicates which packages will be folded when producing stack traces.
+Packages contained in the `exclude` option will be folded. If `only` is provided,
+all packages not contained in this list will be folded. If no options are provided,
+we fold `package:test` and `package:stream_channel` by default.


This document doesn't use the second person elsewhere, so it probably shouldn't do so here. Maybe "By default, frames from the test package and the stream_channel package are folded."

nex3 · 2017-06-16T22:53:00Z

doc/configuration.md

+```yaml
+fold_stack_frames:
+  only:
+   - some_package


Nit: To match other lists in this doc, the -s should be aligned with the beginning of the keys.

nex3 · 2017-06-17T00:30:08Z

test/frontend/test_chain_test.dart

@@ -0,0 +1,103 @@
+// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file


nex3 · 2017-06-17T00:30:23Z

lib/src/frontend/test_chain.dart

@@ -0,0 +1,25 @@
+import 'package:stack_trace/stack_trace.dart';


Don't forget the copyright header.

nex3 · 2017-06-17T00:32:31Z

test/frontend/test_chain_test.dart

+    throw "oh no";
+  });
+}
+""";


Just create this file in a setUp() call.

nex3 · 2017-06-17T00:40:45Z

test/runner/configuration/top_level_error_test.dart

+            ["Invalid option for fold_stack_frames value", "^^^^^^"]));
+    await test.shouldExit(exit_codes.data);
+  });
+


Test what happens if both only and except keys are set.

nex3 · 2017-06-17T00:43:00Z

test/frontend/test_chain_test.dart

+    var test = await runTest(["test.dart"]);
+    var testOutput = (await test.stdoutStream().toList());
+    for (var output in testOutput) {
+      expect(output.contains('package:stream_channel'), isFalse);


I'd use the neverEmits() matcher for this.

Good call. Done.

nex3 · 2017-06-20T20:39:43Z

lib/src/frontend/test_chain.dart

-/// Converts [stackTrace] to a [Chain] following the test's configuration.
-Chain testChain(StackTrace stackTrace, {bool verbose: false}) {
-  var testTrace = currentMapper(stackTrace);
+void configureTestChaining(


Document this.

nex3 · 2017-06-20T20:41:01Z

lib/src/frontend/test_chain.dart

+  if (onlyPackages != null) _onlyPackages = onlyPackages;
+}
+
+Chain terseChain(StackTrace stackTrace, {bool verbose: false}) {


Document this as well.

nex3 · 2017-06-20T20:44:06Z

lib/src/runner/configuration/load.dart

+    }, value: (valueNode) {
+      if (valueNode is! YamlList ||
+          (valueNode as YamlList).firstWhere((value) => value is! String,
+                  orElse: () => null) !=


See section 16.33 of the spec. We don't have to worry about built-in identifiers, but we should enforce the syntactic requirements.

nex3 · 2017-06-20T20:46:21Z

lib/src/runner/configuration/load.dart

+            'Must be a list of strings.',
+            valueNode.span,
+            _source);
+      }


The if statement that throws an exception if valueNode isn't correct.

nex3 · 2017-06-20T20:47:47Z

lib/src/runner/remote_listener.dart

    channel.sink.add({
      "type": "error",
-      "error": RemoteException.serialize(error, stackTrace)
+      "error": RemoteException.serialize(error, testChain(stackTrace))


You'll need to access the verboseTrace flag on the suite-level metadata that was passed in as part of the initial message.

nex3 · 2017-06-20T21:11:51Z

lib/src/runner/configuration/load.dart

+      if (keyNode.value != "only" && keyNode.value != "except") {
+        throw new SourceSpanFormatException(
+            'Must be "only" or "except".', keyNode.span, _source);
+      }


Any statement of the form if (something about value) { throw } should probably be a call to _validate() instead.

nex3 · 2017-06-20T21:13:19Z

lib/src/runner/remote_listener.dart

  }

+  /// Returns a [Set] from a JSON serialized list.
+  static Set<String> _setFromSerializedList(List<String> list) {


Nit: I'd call this _deserializeSet() to match other serialization functions.

nex3 · 2017-06-20T21:14:29Z

lib/src/util/stack_trace_mapper.dart


 import 'dart:async';
+import 'package:collection/collection.dart';
+


This isn't where the newline should go. See Effective Dart.

nex3 · 2017-06-20T21:15:05Z

lib/src/util/stack_trace_mapper.dart

 class StackTraceMapper {
  /// The parsed source map.
-  final Mapping _mapping;
+  Mapping _mapping;


Mention in the documentation that this is initialized lazily in mapStackTrace().

nex3 · 2017-06-20T21:16:08Z

lib/src/util/stack_trace_mapper.dart

-  static Map<String, String> _serializablePackageConfigMap(
+  /// Converts a [packageConfigMap] into a format suitable for JSON
+  /// serialization.
+  static Map<String, String> _serializePackageConfigMap(


These shouldn't be static. Generally, don't make helpers static unless they need to be used in other static methods.

nex3 · 2017-06-20T22:51:38Z

lib/src/frontend/test_chain.dart

    Set<String> onlyPackages}) {
  if (mapper != null) _mapper = mapper;
  if (exceptPackages != null) _exceptPackages = exceptPackages;
  if (onlyPackages != null) _onlyPackages = onlyPackages;


These != null checks are unnecessary.

The null check is necessary since we don't check for nullness in terseChain.

nex3 · 2017-06-20T22:52:05Z

lib/src/runner/configuration.dart

-        _foldTraceExcept = _list(foldTraceExcept),
-        _foldTraceOnly = _list(foldTraceOnly),
+        _foldTraceExcept = foldTraceExcept != null
+            ? new Set.from(_list(foldTraceExcept))


Just add a _set() helper method.

nex3 · 2017-06-20T22:52:57Z

lib/src/runner/configuration.dart

+      foldTraceExcept = new Set.from(other._foldTraceExcept)
+          .union(new Set.from(_foldTraceExcept))
+          .toList();
+    }


I still think nested ifs would be clearer. Broken out, the differences between each condition get lost especially when some conditions duplicate logic in others.

nex3 · 2017-06-20T22:54:26Z

lib/src/runner/configuration/load.dart

+  ///
+  /// The key `except` will correspond to the list of packages to fold.
+  /// The key `only` will correspond to the list of packages to keep in a
+  /// test [Chain].


If the map only has string keys, the type annotation should indicate that.

nex3 · 2017-06-20T22:55:27Z

lib/src/runner/configuration/load.dart

+            'Must be a list of strings.',
+            valueNode.span,
+            _source);
+      }


This still needs to be done.

grouma · 2017-06-22T23:37:40Z

@nex3 I think I have addressed all open comments. This UI is kind of terrible though.

nex3 · 2017-06-22T23:38:21Z

lib/src/runner/configuration.dart

-      foldTraceOnly = other._foldTraceOnly.intersection(_foldTraceOnly);
-    }
-
    var foldTraceExcept = other._foldTraceExcept ?? _foldTraceExcept;


At this point, I don't think it makes sense to set these by default since they'll always either be overridden or end up as null.

Discussed offline. There is a case where we want to default.

nex3 · 2017-06-22T23:38:53Z

lib/src/runner/configuration/load.dart

+
+      _validate(
+          valueNode,
+          "Invalid package identifier",


"identifier" -> "name"

nex3 · 2017-06-22T23:39:07Z

lib/src/runner/configuration/load.dart

-      }
+      _validate(
+          valueNode,
+          "Folded packages must be strings",


These messages need trailing periods.

nex3 · 2017-06-22T23:39:33Z

lib/src/runner/configuration/load.dart

+          "Folded packages must be strings",
+          (valueList) =>
+              valueList is YamlList &&
+              !(valueList).any((value) => value is! String));


valueList.every((value) => value is String)

Same below.

nex3 · 2017-06-22T23:55:01Z

lib/src/runner/configuration/load.dart

          (valueList) =>
              valueList is YamlList &&
-              !(valueList).any((value) => value is! String));
+              (valueList).every((value) => value is String));


The parentheses around valueList are unnecessary.

Originally was casting. Fixed.

These constructors became identical in #650 so we can drop the private copy altogether.

grouma added 7 commits June 16, 2017 12:54

customizable package folding

f458021

fix null errors

fd92345

fix compact reporter tests

3490613

remove unecessary async

512d160

add test chain tests

5e8e0ec

fix grammar

1a45042

add changelog and configuration details

082bb8b

grouma requested a review from nex3 June 16, 2017 22:15

googlebot added the cla: yes label Jun 16, 2017

merge

f2b3e00

nex3 suggested changes Jun 17, 2017

View reviewed changes

grouma added 6 commits June 19, 2017 11:20

doc changes and other minor fixes

ea6db49

check for only one of only or except

1ddcf6f

change configuration logic

7ebb648

sync package resolver

c1d6b48

refactor test_chain file

87ff301

account for null invoker

8de90ce

nex3 suggested changes Jun 20, 2017

View reviewed changes

more review changes

2582c99

nex3 suggested changes Jun 20, 2017

View reviewed changes

grouma added 4 commits June 20, 2017 16:52

more review changes

cd33dc9

package validation

cab199a

better validate

70b7342

different conditionals

9f7c729

nex3 suggested changes Jun 22, 2017

View reviewed changes

more review changes

2d90d0b

nex3 approved these changes Jun 22, 2017

View reviewed changes

grouma added 2 commits June 22, 2017 16:56

fix style

d1513de

merge and prep for release

0c3dc52

grouma merged commit 960bbe6 into dart-lang:master Jun 23, 2017

grouma deleted the remote-mapping branch June 23, 2017 17:13

natebosch added a commit that referenced this pull request Sep 13, 2021

Remove duplicate private constructor

18e9a2a

These constructors became identical in #650 so we can drop the private copy altogether.

natebosch added a commit that referenced this pull request Sep 13, 2021

Remove duplicate private constructor

5d38826

These constructors became identical in #650 so we can drop the private copy altogether.

natebosch mentioned this pull request Sep 13, 2021

Remove duplicate private constructor #1574

Merged

natebosch added a commit that referenced this pull request Sep 14, 2021

Remove duplicate private constructor (#1574)

a612b49

These constructors became identical in #650 so we can drop the private copy altogether.

		@@ -1,3 +1,8 @@
		## 0.12.23

		* Add a `fold_stack_frame` field for `dart_test.yaml`. This will


		### `fold_stack_frames`

		This field indicates which packages will be folded when producing stack traces.

		@@ -0,0 +1,103 @@
		// Copyright (c) 2015, the Dart project authors. Please see the AUTHORS file

		@@ -0,0 +1,25 @@
		import 'package:stack_trace/stack_trace.dart';


		import 'dart:async';
		import 'package:collection/collection.dart';

Configurable package folding #650

Configurable package folding #650

Uh oh!

Conversation

grouma commented Jun 16, 2017

Uh oh!

nex3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment