Skip to content

Commit

Permalink
Simplify package_group unit tests
Browse files Browse the repository at this point in the history 
- Change test assertions to check for the presence/absence of a PackageIdentifier, rather than actually evaluating the Packages that a package_group refers to. This is more straightforward, requires less setup boilerplate, and also allows us to write assertions that talk about packages in external repos, which PackageLoadingTestCase can't evaluate.

- Reflow scratch file contents to follow BUILD formatting.

- Add TODO mentioning a cross-repo test case that'd be nice to have, but isn't worth figuring out how to add.

- Make the negative-everything test case a little more interesting by giving it something to exclude.

- Simplify the positive-everything case's assertion and check that it admits things from other repos.

Work toward #11261.

PiperOrigin-RevId: 475837392
Change-Id: Idcee6afa3a51de569979a2dde619dfa8fd23f1a1
  • Loading branch information
@brandjon @copybara-github
brandjon authored and copybara-github committed Sep 21, 2022
1 parent cea4e30 commit bbe2978
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 72 deletions.
10 changes: 6 additions & 4 deletions src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.License.DistributionType;
import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
Expand Down Expand Up @@ -83,8 +84,8 @@ public PackageGroupContents getPackageSpecifications() {
return packageSpecifications;
}

public boolean contains(Package pkg) {
return packageSpecifications.containsPackage(pkg.getPackageIdentifier());
public boolean contains(PackageIdentifier pkgId) {
return packageSpecifications.containsPackage(pkgId);
}

public List<Label> getIncludes() {
Expand All @@ -110,7 +111,8 @@ public Label getLabel() {
return label;
}

@Override public String getName() {
@Override
public String getName() {
return label.getName();
}

Expand All @@ -136,7 +138,7 @@ public Location getLocation() {

@Override
public String toString() {
return targetKind() + " " + getLabel();
return targetKind() + " " + getLabel();
}

@Override
Expand Down
169 changes: 101 additions & 68 deletions src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
// limitations under the License.
package com.google.devtools.build.lib.packages;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;

import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -27,45 +27,55 @@ public class PackageGroupTest extends PackageLoadingTestCase {

@Test
public void testDoesNotFailHorribly() throws Exception {
scratch.file("fruits/BUILD", "package_group(name = 'apple', packages = ['//random'])");
scratch.file(
"fruits/BUILD",
"package_group(",
" name = 'apple',",
" packages = ['//random'],",
")");
// Note that, for our purposes, the packages listed in the package_group need not exist.

getPackageGroup("fruits", "apple");
}

// Regression test for: "Package group with empty name causes Blaze exception"
@Test
public void testEmptyPackageGroupNameDoesNotThrow() throws Exception {
scratch.file("strawberry/BUILD", "package_group(name = '', packages=[])");
scratch.file(
"strawberry/BUILD", //
"package_group(",
" name = '',",
" packages=[],",
")");

reporter.removeHandler(failFastHandler);
getPackage("strawberry");
// Call getTarget() directly since getPackageGroup() requires a name.
getTarget("//strawberry:BUILD");
assertContainsEvent("package group has invalid name");
}

@Test
public void testAbsolutePackagesWork() throws Exception {
scratch.file(
"fruits/BUILD",
"package_group(name = 'apple',",
" packages = ['//vegetables'])");

scratch.file("vegetables/BUILD");
scratch.file("fruits/vegetables/BUILD");
"package_group(",
" name = 'apple',",
" packages = ['//vegetables'],",
")");

PackageGroup grp = getPackageGroup("fruits", "apple");
assertThat(grp.contains(getPackage("vegetables"))).isTrue();
assertThat(grp.contains(getPackage("fruits/vegetables"))).isFalse();
assertThat(grp.contains(pkgId("vegetables"))).isTrue();
assertThat(grp.contains(pkgId("fruits/vegetables"))).isFalse();
}

@Test
public void testPackagesWithoutDoubleSlashDoNotWork() throws Exception {
scratch.file(
"fruits/BUILD",
"package_group(name = 'apple',",
" packages = ['vegetables'])");

scratch.file("vegetables/BUILD");
scratch.file("fruits/vegetables/BUILD");
"package_group(",
" name = 'apple',",
" packages = ['vegetables'],",
")");

reporter.removeHandler(failFastHandler);
getPackageGroup("fruits", "apple");
Expand All @@ -75,7 +85,11 @@ public void testPackagesWithoutDoubleSlashDoNotWork() throws Exception {
@Test
public void testPackagesWithRepositoryDoNotWork() throws Exception {
scratch.file(
"fruits/BUILD", "package_group(name = 'banana', packages = ['@veggies//:cucumber'])");
"fruits/BUILD",
"package_group(",
" name = 'banana',",
" packages = ['@veggies//:cucumber'],",
")");

reporter.removeHandler(failFastHandler);
getPackageGroup("fruits", "banana");
Expand All @@ -84,22 +98,32 @@ public void testPackagesWithRepositoryDoNotWork() throws Exception {

@Test
public void testAllPackagesInMainRepositoryDoesNotWork() throws Exception {
scratch.file("fruits/BUILD", "package_group(name = 'apple', packages = ['@//...'])");
scratch.file(
"fruits/BUILD", //
"package_group(",
" name = 'apple',",
" packages = ['@//...'],",
")");

reporter.removeHandler(failFastHandler);
getPackageGroup("fruits", "apple");
assertContainsEvent("invalid package name '@//...'");
}

// TODO(brandjon): It'd be nice to include a test here that you can cross repositories via
// `includes`: if package_group //:A includes package_group @repo//:B that has "//foo" in its
// `packages`, then //:A admits package @repo//foo. Unfortunately PackageLoadingTestCase doesn't
// support resolving repos, but similar functionality is tested in
// BzlLoadFunctionTest#testBzlVisibility_enumeratedPackagesMultipleRepos.

@Test
public void testTargetNameAsPackageDoesNotWork1() throws Exception {
scratch.file(
"fruits/BUILD",
"package_group(name = 'apple',",
" packages = ['//vegetables:carrot'])");

scratch.file("vegetables/BUILD");
scratch.file("fruits/vegetables/BUILD");
"package_group(",
" name = 'apple',",
" packages = ['//vegetables:carrot'],",
")");

reporter.removeHandler(failFastHandler);
getPackageGroup("fruits", "apple");
Expand All @@ -108,9 +132,12 @@ public void testTargetNameAsPackageDoesNotWork1() throws Exception {

@Test
public void testTargetNameAsPackageDoesNotWork2() throws Exception {
scratch.file("fruits/BUILD", "package_group(name = 'apple', packages = [':carrot'])");
scratch.file("vegetables/BUILD");
scratch.file("fruits/vegetables/BUILD");
scratch.file(
"fruits/BUILD",
"package_group(",
" name = 'apple',",
" packages = [':carrot'],",
")");

reporter.removeHandler(failFastHandler);
getPackageGroup("fruits", "apple");
Expand All @@ -119,21 +146,22 @@ public void testTargetNameAsPackageDoesNotWork2() throws Exception {

@Test
public void testAllBeneathSpecificationWorks() throws Exception {
scratch.file("fruits/BUILD", "package_group(name = 'maracuja', packages = ['//tropics/...'])");
scratch.file(
"fruits/BUILD",
"package_group(",
" name = 'maracuja',",
" packages = ['//tropics/...'],",
")");

getPackageGroup("fruits", "maracuja");
}

@Test
public void testNegative() throws Exception {
scratch.file("one/BUILD");
scratch.file("two/BUILD");
scratch.file("three/BUILD");
scratch.file("four/BUILD");
scratch.file(
"test/BUILD",
"package_group(",
" name = 'packages',",
" name = 'packages',",
" packages = [",
" '//one',",
" '//two',",
Expand All @@ -143,106 +171,111 @@ public void testNegative() throws Exception {
")");

PackageGroup grp = getPackageGroup("test", "packages");
assertThat(grp.contains(getPackage("one"))).isTrue();
assertThat(grp.contains(getPackage("two"))).isTrue();
assertThat(grp.contains(getPackage("three"))).isFalse();
assertThat(grp.contains(getPackage("four"))).isFalse();
assertThat(grp.contains(pkgId("one"))).isTrue();
assertThat(grp.contains(pkgId("two"))).isTrue();
assertThat(grp.contains(pkgId("three"))).isFalse();
assertThat(grp.contains(pkgId("four"))).isFalse();
}

@Test
public void testNegative_noSubpackages() throws Exception {
scratch.file("pkg/BUILD");
scratch.file("pkg/one/BUILD");
scratch.file("pkg/one/two/BUILD");
scratch.file(
"test/BUILD",
"package_group(",
" name = 'packages',",
" name = 'packages',",
" packages = [",
" '//pkg/...',",
" '-//pkg/one',",
" ],",
")");

PackageGroup grp = getPackageGroup("test", "packages");
assertThat(grp.contains(getPackage("pkg"))).isTrue();
assertThat(grp.contains(getPackage("pkg/one"))).isFalse();
assertThat(grp.contains(getPackage("pkg/one/two"))).isTrue();
assertThat(grp.contains(pkgId("pkg"))).isTrue();
assertThat(grp.contains(pkgId("pkg/one"))).isFalse();
assertThat(grp.contains(pkgId("pkg/one/two"))).isTrue();
}

@Test
public void testNegative_subpackages() throws Exception {
scratch.file("pkg/BUILD");
scratch.file("pkg/one/BUILD");
scratch.file("pkg/one/two/BUILD");
scratch.file(
"test/BUILD",
"package_group(",
" name = 'packages',",
" name = 'packages',",
" packages = [",
" '//pkg/...',",
" '-//pkg/one/...',",
" ],",
")");

PackageGroup grp = getPackageGroup("test", "packages");
assertThat(grp.contains(getPackage("pkg"))).isTrue();
assertThat(grp.contains(getPackage("pkg/one"))).isFalse();
assertThat(grp.contains(getPackage("pkg/one/two"))).isFalse();
assertThat(grp.contains(pkgId("pkg"))).isTrue();
assertThat(grp.contains(pkgId("pkg/one"))).isFalse();
assertThat(grp.contains(pkgId("pkg/one/two"))).isFalse();
}

@Test
public void testNegative_everything() throws Exception {
scratch.file("pkg/BUILD");
scratch.file("pkg/one/BUILD");
scratch.file("pkg/one/two/BUILD");
scratch.file(
"test/BUILD",
"package_group(",
" name = 'packages',",
" name = 'packages',",
" packages = [",
" '//pkg/one',",
" '-//...',",
" ],",
")");

PackageGroup grp = getPackageGroup("test", "packages");
assertThat(grp.contains(getPackage("pkg"))).isFalse();
assertThat(grp.contains(getPackage("pkg/one"))).isFalse();
assertThat(grp.contains(getPackage("pkg/one/two"))).isFalse();
assertThat(grp.contains(pkgId("pkg"))).isFalse();
assertThat(grp.contains(pkgId("pkg/one"))).isFalse();
assertThat(grp.contains(pkgId("pkg/one/two"))).isFalse();
}

@Test
public void testEverythingSpecificationWorks() throws Exception {
scratch.file("fruits/BUILD", "package_group(name = 'mango', packages = ['//...'])");
PackageGroup packageGroup = getPackageGroup("fruits", "mango");
assertThat(
packageGroup.getPackageSpecifications().containedPackages().collect(toImmutableList()))
scratch.file(
"fruits/BUILD", //
"package_group(",
" name = 'mango',",
" packages = ['//...'],",
")");
PackageGroup grp = getPackageGroup("fruits", "mango");

// Assert that we're actually using the "everything" package specification, and assert that this
// means we include packages from the main repo but also from other repos.
assertThat(grp.getContainedPackages())
.containsExactly(PackageSpecification.everything().toString());
assertThat(grp.contains(pkgId("pkg"))).isTrue();
assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue();
}

@Test
public void testDuplicatePackage() throws Exception {
scratch.file("pkg/BUILD");
scratch.file("one/two/BUILD");

scratch.file(
"test/BUILD",
"package_group(",
" name = 'packages',",
" name = 'packages',",
" packages = [",
" '//one/two',",
" '//one/two',",
" ],",
")");

PackageGroup grp = getPackageGroup("test", "packages");
assertThat(grp.contains(getPackage("one/two"))).isTrue();
assertThat(grp.contains(pkgId("one/two"))).isTrue();
}

/** Convenience method for obtaining a PackageIdentifier. */
private PackageIdentifier pkgId(String packageName) throws Exception {
return PackageIdentifier.createUnchecked(/*repository=*/ "", packageName);
}

private Package getPackage(String packageName) throws Exception {
return getTarget("//" + packageName + ":BUILD").getPackage();
/** Convenience method for obtaining a PackageIdentifier outside the main repo. */
private PackageIdentifier pkgId(String repoName, String packageName) throws Exception {
return PackageIdentifier.createUnchecked(repoName, packageName);
}

/** Evaluates and returns the requested package_group target. */
private PackageGroup getPackageGroup(String pkg, String name) throws Exception {
return (PackageGroup) getTarget("//" + pkg + ":" + name);
}
Expand Down

0 comments on commit bbe2978

Please sign in to comment.