Skip to content

Triage more solver tests #1842

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

Merged
merged 5 commits into from
Mar 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/src/global_packages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import 'package_name.dart';
import 'pubspec.dart';
import 'sdk.dart' as sdk;
import 'solver.dart';
import 'solver/incompatibility_cause.dart';
import 'source/cached.dart';
import 'source/git.dart';
import 'source/hosted.dart';
Expand Down Expand Up @@ -175,7 +176,18 @@ class GlobalPackages {
//
// TODO(nweiz): If this produces a SolveFailure that's caused by [dep] not
// being available, report that as a [dataError].
var result = await resolveVersions(SolveType.GET, cache, root);
SolveResult result;
try {
result = await resolveVersions(SolveType.GET, cache, root);
} on SolveFailure catch (error) {
for (var incompatibility
in error.incompatibility.externalIncompatibilities) {
if (incompatibility.cause != IncompatibilityCause.noVersions) continue;
if (incompatibility.terms.single.package.name != dep.name) continue;
dataError(error.toString());
}
rethrow;
}

result.showReport(SolveType.GET);

Expand Down
6 changes: 4 additions & 2 deletions lib/src/solver/failure.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class SolveFailure implements ApplicationException {
/// If multiple [PackageNotFoundException]s caused the error, it's undefined
/// which one is returned.
PackageNotFoundException get packageNotFound {
for (var cause in incompatibility.externalCauses) {
for (var incompatibility in incompatibility.externalIncompatibilities) {
var cause = incompatibility.cause;
if (cause is PackageNotFoundCause) return cause.exception;
}
return null;
Expand Down Expand Up @@ -94,7 +95,8 @@ class _Writer {
var hasFlutterCause = false;
var hasDartSdkCause = false;
var hasFlutterSdkCause = false;
for (var cause in _root.externalCauses) {
for (var incompatibility in _root.externalIncompatibilities) {
var cause = incompatibility.cause;
if (cause.isFlutter) hasFlutterCause = true;
if (cause is SdkCause) {
if (cause.isFlutter) {
Expand Down
12 changes: 6 additions & 6 deletions lib/src/solver/incompatibility.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ class Incompatibility {
bool get isFailure =>
terms.isEmpty || (terms.length == 1 && terms.first.package.isRoot);

/// Returns the causes of all external incompatibilities in this
/// incompatibility's derivation graph.
Iterable<IncompatibilityCause> get externalCauses sync* {
/// Returns all external incompatibilities in this incompatibility's
/// derivation graph.
Iterable<Incompatibility> get externalIncompatibilities sync* {
if (cause is ConflictCause) {
var cause = this.cause as ConflictCause;
yield* cause.conflict.externalCauses;
yield* cause.other.externalCauses;
yield* cause.conflict.externalIncompatibilities;
yield* cause.other.externalIncompatibilities;
} else {
yield cause;
yield this;
}
}

Expand Down
23 changes: 18 additions & 5 deletions lib/src/solver/package_lister.dart
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ class PackageLister {
? _matchesFlutterSdkConstraint(pubspec)
: _matchesDartSdkConstraint(pubspec);

if (allowsSdk(await _source.describe(versions[index]))) return null;
if (allowsSdk(await _describeSafe(versions[index]))) return null;

var bounds = await _findBounds(index, (pubspec) => !allowsSdk(pubspec));
var incompatibleVersions = new VersionRange(
Expand All @@ -300,7 +300,7 @@ class PackageLister {
var sdkConstraint = await foldAsync(
slice(versions, bounds.first, bounds.last + 1), VersionConstraint.empty,
(previous, version) async {
var pubspec = await _source.describe(version);
var pubspec = await _describeSafe(version);
return previous.union(
flutter ? pubspec.flutterSdkConstraint : pubspec.dartSdkConstraint);
});
Expand All @@ -320,13 +320,13 @@ class PackageLister {

var first = start - 1;
while (first > 0) {
if (!match(await _source.describe(versions[first]))) break;
if (!match(await _describeSafe(versions[first]))) break;
first--;
}

var last = start + 1;
while (last < versions.length) {
if (!match(await _source.describe(versions[last]))) break;
if (!match(await _describeSafe(versions[last]))) break;
last++;
}

Expand All @@ -350,7 +350,7 @@ class PackageLister {
for (var id in upper
? versions.skip(index + 1)
: versions.reversed.skip(versions.length - index)) {
var pubspec = await _source.describe(id);
var pubspec = await _describeSafe(id);

// The upper bound is exclusive and so is the first package with a
// different dependency. The lower bound is inclusive, and so is the last
Expand Down Expand Up @@ -381,6 +381,19 @@ class PackageLister {
return bounds;
}

/// Returns the pubspec for [id], or an empty pubpsec matching [id] if the
/// real pubspec for [id] fails to load for any reason.
///
/// This makes the bounds-finding logic resilient to broken pubspecs while
/// keeping the actual error handling in a central location.
Future<Pubspec> _describeSafe(PackageId id) async {
try {
return await _source.describe(id);
} catch (_) {
return new Pubspec(id.name, version: id.version);
}
}

/// Returns whether [pubspec]'s Dart SDK constraint matches the current Dart
/// SDK version.
bool _matchesDartSdkConstraint(Pubspec pubspec) =>
Expand Down
36 changes: 0 additions & 36 deletions test/get/path/shared_dependency_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE d.file.

@Skip()

import 'package:test/test.dart';

import 'package:path/path.dart' as path;
Expand Down Expand Up @@ -73,38 +71,4 @@ main() {
await d.appPackagesFile(
{"foo": "../foo", "bar": "../bar", "shared": "../shared"}).validate();
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to these tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained in the commit message.

test("shared dependency with absolute and relative path", () async {
await d.dir("shared",
[d.libDir("shared"), d.libPubspec("shared", "0.0.1")]).create();

await d.dir("foo", [
d.libDir("foo"),
d.libPubspec("foo", "0.0.1", deps: {
"shared": {"path": "../shared"}
})
]).create();

await d.dir("bar", [
d.libDir("bar"),
d.libPubspec("bar", "0.0.1", deps: {
"shared": {"path": path.join(d.sandbox, "shared")}
})
]).create();

await d.dir(appPath, [
d.appPubspec({
"foo": {"path": "../foo"},
"bar": {"path": "../bar"}
})
]).create();

await pubGet();

await d.appPackagesFile({
"foo": "../foo",
"bar": "../bar",
"shared": path.join(d.sandbox, "shared")
}).validate();
});
}
9 changes: 4 additions & 5 deletions test/global/activate/empty_constraint_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

@Skip()

import 'package:test/test.dart';

import 'package:pub/src/exit_codes.dart' as exit_codes;
Expand All @@ -19,9 +17,10 @@ main() {

await runPub(
args: ["global", "activate", "foo", ">1.1.0"],
error: """
Package foo has no versions that match >1.1.0 derived from:
- pub global activate depends on version >1.1.0""",
error: equalsIgnoringWhitespace("""
Because pub global activate depends on foo >1.1.0 which doesn't match
any versions, version solving failed.
"""),
exitCode: exit_codes.DATA);
});
}
2 changes: 0 additions & 2 deletions test/hosted/fail_gracefully_on_url_resolve_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

@Skip()

import 'package:test/test.dart';

import 'package:pub/src/exit_codes.dart' as exit_codes;
Expand Down
23 changes: 12 additions & 11 deletions test/hosted/offline_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

@Skip()

import 'package:test/test.dart';

import 'package:pub/src/exit_codes.dart' as exit_codes;
Expand Down Expand Up @@ -65,9 +63,10 @@ main() {
await pubCommand(command,
args: ['--offline'],
exitCode: exit_codes.UNAVAILABLE,
error: "Could not find package foo in cache.\n"
"Depended on by:\n"
"- myapp");
error: equalsIgnoringWhitespace("""
Because myapp depends on foo any which doesn't exist (could not find
package foo in cache), version solving failed.
"""));
});

test('fails gracefully if no cached versions match', () async {
Expand All @@ -81,9 +80,10 @@ main() {
await d.appDir({"foo": ">2.0.0"}).create();

await pubCommand(command,
args: ['--offline'],
error: "Package foo has no versions that match >2.0.0 derived from:\n"
"- myapp depends on version >2.0.0");
args: ['--offline'], error: equalsIgnoringWhitespace("""
Because myapp depends on foo >2.0.0 which doesn't match any
versions, version solving failed.
"""));
});

test(
Expand All @@ -99,9 +99,10 @@ main() {
await pubCommand(command,
args: ['--offline'],
exitCode: exit_codes.UNAVAILABLE,
error: "Could not find package foo in cache.\n"
"Depended on by:\n"
"- myapp");
error: equalsIgnoringWhitespace("""
Because myapp depends on foo any which doesn't exist (could not find
package foo in cache), version solving failed.
"""));
});

test('downgrades to the version in the cache if necessary', () async {
Expand Down
52 changes: 27 additions & 25 deletions test/sdk_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ main() {
.file("$appPath/pubspec.lock",
allOf([isNot(contains("0.0.1")), contains("0.0.2")]))
.validate();
}, skip: true);
});

group("fails if", () {
test("the version constraint doesn't match", () async {
Expand All @@ -93,33 +93,33 @@ main() {
}).create();
await pubCommand(command,
environment: {'FLUTTER_ROOT': p.join(d.sandbox, 'flutter')},
error: 'Package foo has no versions that match >=1.0.0 <2.0.0 '
'derived from:\n'
'- myapp depends on version ^1.0.0');
error: equalsIgnoringWhitespace("""
Because myapp depends on foo ^1.0.0 from sdk which doesn't match
any versions, version solving failed.
"""));
});

test("the SDK is unknown", () async {
await d.appDir({
"foo": {"sdk": "unknown"}
}).create();
await pubCommand(command,
error: 'Unknown SDK "unknown".\n'
'Depended on by:\n'
'- myapp',
exitCode: exit_codes.UNAVAILABLE);
await pubCommand(command, error: equalsIgnoringWhitespace("""
Because myapp depends on foo any from sdk which doesn't exist
(unknown SDK "unknown"), version solving failed.
"""), exitCode: exit_codes.UNAVAILABLE);
});

test("the SDK is unavailable", () async {
await d.appDir({
"foo": {"sdk": "flutter"}
}).create();
await pubCommand(command,
error: 'The Flutter SDK is not available.\n'
'Flutter users should run `flutter packages get` instead of '
'`pub get`.\n'
'Depended on by:\n'
'- myapp',
exitCode: exit_codes.UNAVAILABLE);
await pubCommand(command, error: equalsIgnoringWhitespace("""
Because myapp depends on foo any from sdk which doesn't exist (the
Flutter SDK is not available), version solving failed.

Flutter users should run `flutter packages get` instead of `pub
get`.
"""), exitCode: exit_codes.UNAVAILABLE);
});

test("the SDK doesn't contain the package", () async {
Expand All @@ -128,22 +128,24 @@ main() {
}).create();
await pubCommand(command,
environment: {'FLUTTER_ROOT': p.join(d.sandbox, 'flutter')},
error: 'Could not find package bar in the Flutter SDK.\n'
'Depended on by:\n'
'- myapp',
error: equalsIgnoringWhitespace("""
Because myapp depends on bar any from sdk which doesn't exist
(could not find package bar in the Flutter SDK), version solving
failed.
"""),
exitCode: exit_codes.UNAVAILABLE);
});

test("the Dart SDK doesn't contain the package", () async {
await d.appDir({
"bar": {"sdk": "dart"}
}).create();
await pubCommand(command,
error: 'Could not find package bar in the Dart SDK.\n'
'Depended on by:\n'
'- myapp',
exitCode: exit_codes.UNAVAILABLE);
await pubCommand(command, error: equalsIgnoringWhitespace("""
Because myapp depends on bar any from sdk which doesn't exist
(could not find package bar in the Dart SDK), version solving
failed.
"""), exitCode: exit_codes.UNAVAILABLE);
});
}, skip: true);
});
});
}
15 changes: 9 additions & 6 deletions test/unknown_source_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE d.file.

@Skip()

import 'dart:convert';

import 'package:test/test.dart';
Expand All @@ -18,8 +16,10 @@ main() {
"foo": {"bad": "foo"}
}).create();

await pubCommand(command,
error: 'Package myapp depends on foo from unknown source "bad".');
await pubCommand(command, error: equalsIgnoringWhitespace("""
Because myapp depends on foo from unknown source "bad", version solving
failed.
"""));
});

test(
Expand All @@ -36,8 +36,11 @@ main() {
"foo": {"path": "../foo"}
}).create();

await pubCommand(command,
error: 'Package foo depends on bar from unknown source "bad".');
await pubCommand(command, error: equalsIgnoringWhitespace("""
Because every version of foo from path depends on bar from unknown
source "bad", foo from path is forbidden.
So, because myapp depends on foo from path, version solving failed.
"""));
});

test('ignores unknown source in lockfile', () async {
Expand Down
Loading