From 123f5461a27ed768d518a7c476b16e09f5f69bd5 Mon Sep 17 00:00:00 2001
From: Jacob MacDonald <jakemac@google.com>
Date: Tue, 14 Jan 2020 11:29:03 -0800
Subject: [PATCH] Fix a bug in build_test where the resolve* apis would leak
 unhandled async errors (#2599)

Fixes https://github.com/dart-lang/build/issues/2596

Specifically if the `action` callback throws any exceptions then we would leak those as unhandled async errors.

We can't await the call to `runBuilder` because that would block on the `tearDown` future which would hang tests.

The solution is to just swallow all errors from the unawaited future, and rely on the `onDone` Future to report most errors. We try/catch the action callback and forward the errors to the `onDone` completer.
---
 .../test/asset/file_based_test.dart            |  4 ++--
 build_test/CHANGELOG.md                        |  5 +++--
 build_test/lib/src/resolve_source.dart         | 18 ++++++++++++++----
 build_test/pubspec.yaml                        |  2 +-
 build_test/test/_files/example_lib.dart        |  2 ++
 build_test/test/_files/example_part.dart       |  1 +
 build_test/test/resolve_source_test.dart       | 12 ++++++++++++
 7 files changed, 35 insertions(+), 9 deletions(-)
 create mode 100644 build_test/test/_files/example_part.dart

diff --git a/build_runner_core/test/asset/file_based_test.dart b/build_runner_core/test/asset/file_based_test.dart
index 6b6e78448..b1260a3a6 100644
--- a/build_runner_core/test/asset/file_based_test.dart
+++ b/build_runner_core/test/asset/file_based_test.dart
@@ -102,8 +102,8 @@ void main() {
 
     test('can read from the SDK', () async {
       expect(
-          await reader
-              .canRead(makeAssetId(r'$sdk|lib/dev_compiler/amd/dart_sdk.js')),
+          await reader.canRead(
+              makeAssetId(r'$sdk|lib/dev_compiler/kernel/amd/dart_sdk.js')),
           true);
     });
   });
diff --git a/build_test/CHANGELOG.md b/build_test/CHANGELOG.md
index ab0109c53..133b8ecf5 100644
--- a/build_test/CHANGELOG.md
+++ b/build_test/CHANGELOG.md
@@ -1,6 +1,7 @@
-## 0.10.12-dev
+## 0.10.12
 
-- Internal changes.
+- Fix a bug with the `resolve*` apis where they would leak unhandled async
+  errors to client code if the provided action callback threw an error.
 
 ## 0.10.11
 
diff --git a/build_test/lib/src/resolve_source.dart b/build_test/lib/src/resolve_source.dart
index 381ab6711..be083970f 100644
--- a/build_test/lib/src/resolve_source.dart
+++ b/build_test/lib/src/resolve_source.dart
@@ -193,14 +193,21 @@ Future<T> _resolveAssets<T>(
     sourceAssets: inputAssets,
     rootPackage: rootPackage,
   );
-  // We don't care about the results of this build.
+  // We don't care about the results of this build, but we also can't await
+  // it because that would block on the `tearDown` of the `resolveBuilder`.
+  //
+  // We also dont want to leak errors as unhandled async errors so we swallow
+  // them here.
+  //
+  // Errors will still be reported through the resolver itself as well as the
+  // `onDone` future that we return.
   unawaited(runBuilder(
     resolveBuilder,
     inputAssets.keys,
     MultiAssetReader([inMemory, assetReader]),
     InMemoryAssetWriter(),
     resolvers ?? defaultResolvers,
-  ));
+  ).catchError((_) {}));
   return resolveBuilder.onDone.future;
 }
 
@@ -220,8 +227,11 @@ class _ResolveSourceBuilder<T> implements Builder {
   @override
   Future<void> build(BuildStep buildStep) async {
     if (_resolverFor != buildStep.inputId) return;
-    var result = await _action(buildStep.resolver);
-    onDone.complete(result);
+    try {
+      onDone.complete(await _action(buildStep.resolver));
+    } catch (e, s) {
+      onDone.completeError(e, s);
+    }
     await _tearDown;
   }
 
diff --git a/build_test/pubspec.yaml b/build_test/pubspec.yaml
index 978d52fc6..599d74e57 100644
--- a/build_test/pubspec.yaml
+++ b/build_test/pubspec.yaml
@@ -1,6 +1,6 @@
 name: build_test
 description: Utilities for writing unit tests of Builders.
-version: 0.10.12-dev
+version: 0.10.12
 homepage: https://github.com/dart-lang/build/tree/master/build_test
 
 environment:
diff --git a/build_test/test/_files/example_lib.dart b/build_test/test/_files/example_lib.dart
index bc969eb0f..899bdbe4c 100644
--- a/build_test/test/_files/example_lib.dart
+++ b/build_test/test/_files/example_lib.dart
@@ -4,4 +4,6 @@
 
 library example_lib;
 
+part 'example_part.dart';
+
 class Example {}
diff --git a/build_test/test/_files/example_part.dart b/build_test/test/_files/example_part.dart
new file mode 100644
index 000000000..b66457704
--- /dev/null
+++ b/build_test/test/_files/example_part.dart
@@ -0,0 +1 @@
+part of 'example_lib.dart';
diff --git a/build_test/test/resolve_source_test.dart b/build_test/test/resolve_source_test.dart
index 1a1a6b65a..2d89f638a 100644
--- a/build_test/test/resolve_source_test.dart
+++ b/build_test/test/resolve_source_test.dart
@@ -119,6 +119,18 @@ void main() {
       expect(libExample.getType('Example'), isNotNull);
     });
   });
+
+  group('error handling', () {
+    test('getting the library for a part file', () async {
+      var partAsset = AssetId('build_test', 'test/_files/example_part.dart');
+      await resolveAsset(partAsset, (resolver) async {
+        expect(
+            () => resolver.libraryFor(partAsset),
+            throwsA(isA<NonLibraryAssetException>()
+                .having((e) => e.assetId, 'assetId', partAsset)));
+      });
+    });
+  });
 }
 
 String _toStringId(InterfaceType t) =>