Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug in build_test where the resolve* apis would leak unhandled async errors #2599

Merged
merged 4 commits into from
Jan 14, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions build_test/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down
31 changes: 21 additions & 10 deletions build_test/lib/src/resolve_source.dart
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,22 @@ Future<T> _resolveAssets<T>(
sourceAssets: inputAssets,
rootPackage: rootPackage,
);
// We don't care about the results of this build.
unawaited(runBuilder(
resolveBuilder,
inputAssets.keys,
MultiAssetReader([inMemory, assetReader]),
InMemoryAssetWriter(),
resolvers ?? defaultResolvers,
));
// 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 unhandled async errors so we swallow them.
jakemac53 marked this conversation as resolved.
Show resolved Hide resolved
//
// Errors will still be reported through the resolver itself as well as the
// `onDone` future that we return.
unawaited(runZoned(
() => runBuilder(
resolveBuilder,
inputAssets.keys,
MultiAssetReader([inMemory, assetReader]),
InMemoryAssetWriter(),
resolvers ?? defaultResolvers,
),
onError: (_) {}));
return resolveBuilder.onDone.future;
}

Expand All @@ -220,8 +228,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));
jakemac53 marked this conversation as resolved.
Show resolved Hide resolved
} catch (e, s) {
onDone.completeError(e, s);
}
await _tearDown;
}

Expand Down
2 changes: 1 addition & 1 deletion build_test/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
2 changes: 2 additions & 0 deletions build_test/test/_files/example_lib.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,6 @@

library example_lib;

part 'example_part.dart';

class Example {}
1 change: 1 addition & 0 deletions build_test/test/_files/example_part.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
part of 'example_lib.dart';
12 changes: 12 additions & 0 deletions build_test/test/resolve_source_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down