Add test coverage for label visitation over computed defaults in AggregatingAttributeMapper.

Recall that computed defaults have the ability to read all nonconfigurable attributes and whatever configurable attributes they declare in their constructors.

I mistakenly thought that AggregatingAttributeMapper.visitLabels was skipping computed defaults that didn't declare configurable deps.

While everything was, in fact, fine, this cl properly enforces expectations.

In the process, I created a custom rule class in AggregatingAttributeMapperTest, which forced AbstractAttributeMapperTest to inherit BuildViewTestCase. That offered some code cleanup opportunity that makes up the rest of this cl.

--
MOS_MIGRATED_REVID=122838948

4 files changed, 121 insertions, 35 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java
index 4ecd5ed560..942e1bba29 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java
@@ -20,10 +20,9 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
+import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
 import com.google.devtools.build.lib.packages.AbstractAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.AttributeContainer;
@@ -32,11 +31,7 @@ import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
-import com.google.devtools.build.lib.packages.util.PackageFactoryApparatus;
 import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.testutil.FoundationTestCase;
-import com.google.devtools.build.lib.testutil.Scratch;
-import com.google.devtools.build.lib.vfs.Path;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -49,9 +44,8 @@ import java.util.List;
  * Unit tests for {@link AbstractAttributeMapper}.
  */
 @RunWith(JUnit4.class)
-public class AbstractAttributeMapperTest extends FoundationTestCase {
+public class AbstractAttributeMapperTest extends BuildViewTestCase {
 
-  private Package pkg;
   protected Rule rule;
   protected AbstractAttributeMapper mapper;
 
@@ -62,23 +56,14 @@ public class AbstractAttributeMapperTest extends FoundationTestCase {
     }
   }
 
-  protected Rule createRule(String pkgPath, String ruleName, String... ruleDef) throws Exception  {
-    Scratch scratch = new Scratch();
-    EventCollectionApparatus events = new EventCollectionApparatus();
-    PackageFactoryApparatus packages = new PackageFactoryApparatus(events.reporter());
-
-    Path buildFile = scratch.file(pkgPath + "/BUILD", ruleDef);
-    pkg = packages.createPackage(pkgPath, buildFile);
-    return pkg.getRule(ruleName);
-  }
-
   @Before
   public final void initializeRuleAndMapper() throws Exception {
-    rule = createRule("x", "myrule",
+    rule = scratchRule("p", "myrule",
         "cc_binary(name = 'myrule',",
         "          srcs = ['a', 'b', 'c'])");
     RuleClass ruleClass = rule.getRuleClassObject();
-    mapper = new TestMapper(pkg, ruleClass, rule.getLabel(), rule.getAttributeContainer());
+    mapper =
+        new TestMapper(rule.getPackage(), ruleClass, rule.getLabel(), rule.getAttributeContainer());
   }
 
   @Test
@@ -89,6 +74,10 @@ public class AbstractAttributeMapperTest extends FoundationTestCase {
 
   @Test
   public void testPackageDefaultProperties() throws Exception {
+    rule = scratchRule("a", "myrule",
+        "cc_binary(name = 'myrule',",
+        "          srcs = ['a', 'b', 'c'])");
+    Package pkg = rule.getPackage();
     assertEquals(pkg.getDefaultHdrsCheck(), mapper.getPackageDefaultHdrsCheck());
     assertEquals(pkg.getDefaultTestOnly(), mapper.getPackageDefaultTestOnly());
     assertEquals(pkg.getDefaultDeprecation(), mapper.getPackageDefaultDeprecation());
@@ -155,8 +144,7 @@ public class AbstractAttributeMapperTest extends FoundationTestCase {
   public void testVisitation() throws Exception {
     VisitationRecorder recorder = new VisitationRecorder("srcs");
     mapper.visitLabels(recorder);
-    assertThat(recorder.labelsVisited)
-        .containsExactlyElementsIn(ImmutableList.of("//x:a", "//x:b", "//x:c"));
+    assertThat(recorder.labelsVisited).containsExactly("//p:a", "//p:b", "//p:c");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
index e9f4305d29..318fe7d124 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
@@ -14,18 +14,32 @@
 package com.google.devtools.build.lib.analysis.select;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.syntax.Type.STRING;
 import static org.junit.Assert.assertNull;
 
+import com.google.common.base.Joiner;
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.analysis.BaseRuleClasses;
+import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.RuleDefinition;
+import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.testutil.TestConstants;
+import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.testutil.UnknownRuleConfiguredTarget;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -55,7 +69,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
    */
   @Test
   public void testGetPossibleValuesDirectAttribute() throws Exception {
-    Rule rule = createRule("a", "myrule",
+    Rule rule = scratchRule("a", "myrule",
         "sh_binary(name = 'myrule',",
         "          srcs = ['a.sh'])");
     assertThat(AggregatingAttributeMapper.of(rule).visitAttribute("srcs", BuildType.LABEL_LIST))
@@ -68,7 +82,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
    */
   @Test
   public void testGetPossibleValuesConfigurableAttribute() throws Exception {
-    Rule rule = createRule("a", "myrule",
+    Rule rule = scratchRule("a", "myrule",
         "sh_binary(name = 'myrule',",
         "          srcs = select({",
         "              '//conditions:a': ['a.sh'],",
@@ -85,7 +99,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
 
   @Test
   public void testGetPossibleValuesWithConcatenatedSelects() throws Exception {
-    Rule rule = createRule("a", "myrule",
+    Rule rule = scratchRule("a", "myrule",
         "sh_binary(name = 'myrule',",
         "          srcs = select({",
         "                  '//conditions:a1': ['a1.sh'],",
@@ -116,7 +130,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
       ruleDef.append(String.format(pattern, c, Character.toUpperCase(c)));
     }
     ruleDef.append(")");
-    Rule rule = createRule("a", "gen", ruleDef.toString());
+    Rule rule = scratchRule("a", "gen", ruleDef.toString());
     // Naive evaluation would visit 2^26 cases and either overflow memory or timeout the test.
     assertThat(AggregatingAttributeMapper.of(rule).visitAttribute("cmd", Type.STRING))
         .containsExactly("abcdefghijklmnopqrstuvwxyz", "ABCDEFGHIJKLMNOPQRSTUVWXYZ");
@@ -128,7 +142,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
    */
   @Test
   public void testVisitationConfigurableAttribute() throws Exception {
-    Rule rule = createRule("a", "myrule",
+    Rule rule = scratchRule("a", "myrule",
         "sh_binary(name = 'myrule',",
         "          srcs = select({",
         "              '//conditions:a': ['a.sh'],",
@@ -146,7 +160,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
 
   @Test
   public void testVisitationWithDefaultValues() throws Exception {
-    Rule rule = createRule("a", "myrule",
+    Rule rule = scratchRule("a", "myrule",
         "cc_binary(name = 'myrule',",
         "    srcs = [],",
         "    malloc = select({",
@@ -163,7 +177,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
 
   @Test
   public void testGetReachableLabels() throws Exception {
-    Rule rule = createRule("x", "main",
+    Rule rule = scratchRule("x", "main",
         "cc_binary(",
         "    name = 'main',",
         "    srcs = select({",
@@ -194,7 +208,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
 
   @Test
   public void testGetReachableLabelsWithDefaultValues() throws Exception {
-    Rule rule = createRule("a", "myrule",
+    Rule rule = scratchRule("a", "myrule",
         "cc_binary(name = 'myrule',",
         "    srcs = [],",
         "    malloc = select({",
@@ -212,7 +226,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
     if (TestConstants.THIS_IS_BAZEL) {
       return;
     }
-    Rule rule = createRule("x", "main",
+    Rule rule = scratchRule("x", "main",
         "java_binary(",
         "    name = 'main',",
         "    srcs = ['main.java'])");
@@ -222,4 +236,85 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
     assertThat(mapper.checkForDuplicateLabels(launcherAttribute))
         .containsExactlyElementsIn(ImmutableList.of());
   }
+
+  /**
+   * Custom rule to support testing over computed defaults.
+   */
+  public static final class RuleWithComputedDefaults
+      implements RuleDefinition, RuleConfiguredTargetFactory {
+    @Override
+    public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
+      return builder
+          .add(attr("configurable1", STRING))
+          .add(attr("configurable2", STRING))
+          .add(attr("nonconfigurable", STRING).nonconfigurable("that's the point"))
+          .add(attr("$computed_default_with_configurable_deps", STRING).value(
+              new Attribute.ComputedDefault("configurable1", "configurable2") {
+                @Override
+                public Object getDefault(AttributeMap rule) {
+                  return Joiner.on(" ").join(
+                      rule.get("configurable1", STRING),
+                      rule.get("configurable2", STRING),
+                      rule.get("nonconfigurable", STRING)
+                  );
+                }
+              }))
+          .add(attr("$computed_default_without_configurable_deps", STRING).value(
+              new Attribute.ComputedDefault() {
+                @Override
+                public Object getDefault(AttributeMap rule) {
+                  return rule.get("nonconfigurable", STRING);
+                }
+              }))
+          .build();
+    }
+
+    @Override
+    public Metadata getMetadata() {
+      return RuleDefinition.Metadata.builder()
+          .name("rule_with_computed_defaults")
+          .ancestors(BaseRuleClasses.BaseRule.class)
+          .factoryClass(UnknownRuleConfiguredTarget.class)
+          .build();
+    }
+
+    @Override
+    public ConfiguredTarget create(RuleContext ruleContext) throws InterruptedException {
+      throw new UnsupportedOperationException();
+    }
+  }
+
+  @Override
+  protected ConfiguredRuleClassProvider getRuleClassProvider() {
+    ConfiguredRuleClassProvider.Builder builder =
+        new ConfiguredRuleClassProvider.Builder()
+            .addRuleDefinition(new RuleWithComputedDefaults());
+    TestRuleClassProvider.addStandardRules(builder);
+    return builder.build();
+  }
+
+  @Test
+  public void testComputedDefaultWithConfigurableDeps() throws Exception {
+    Rule rule = scratchRule("x", "bb",
+        "rule_with_computed_defaults(",
+        "    name = 'bb',",
+        "    configurable1 = select({':a': 'of', ':b': 'from'}),",
+        "    configurable2 = select({':a': 'this', ':b': 'the'}),",
+        "    nonconfigurable = 'bottom')");
+    assertThat(AggregatingAttributeMapper.of(rule)
+        .visitAttribute("$computed_default_with_configurable_deps", STRING))
+        .containsExactly("of this bottom", "from this bottom", "of the bottom", "from the bottom");
+  }
+
+  @Test
+  public void testComputedDefaultWithoutConfigurableDeps() throws Exception {
+    Rule rule = scratchRule("x", "bb",
+        "rule_with_computed_defaults(",
+        "    name = 'bb',",
+        "    nonconfigurable = 'swim up')");
+    assertThat(AggregatingAttributeMapper.of(rule)
+        .visitAttribute("$computed_default_without_configurable_deps", STRING))
+        .containsExactly("swim up");
+  }
+
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/NonconfigurableAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/NonconfigurableAttributeMapperTest.java
index a634c1108a..79a1eb1d34 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/NonconfigurableAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/NonconfigurableAttributeMapperTest.java
@@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.syntax.Type;
 
 import org.junit.Before;
@@ -31,9 +32,11 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class NonconfigurableAttributeMapperTest extends AbstractAttributeMapperTest {
 
+  private Rule rule;
+
   @Before
   public final void createRule() throws Exception {
-    rule = createRule("x", "myrule",
+    rule = scratchRule("x", "myrule",
         "cc_binary(",
         "    name = 'myrule',",
         "    srcs = ['a', 'b', 'c'],",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
index b0d4354af8..16ebc6c87c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
@@ -48,7 +48,7 @@ public class RawAttributeMapperTest extends AbstractAttributeMapperTest {
   }
 
   private Rule setupGenRule() throws Exception {
-    return createRule("x", "myrule",
+    return scratchRule("x", "myrule",
         "sh_binary(",
         "    name = 'myrule',",
         "    srcs = select({",
@@ -127,7 +127,7 @@ public class RawAttributeMapperTest extends AbstractAttributeMapperTest {
 
   @Test
   public void testGetMergedValues() throws Exception {
-    Rule rule = createRule("x", "myrule",
+    Rule rule = scratchRule("x", "myrule",
         "sh_binary(",
         "    name = 'myrule',",
         "    srcs = select({",
@@ -144,7 +144,7 @@ public class RawAttributeMapperTest extends AbstractAttributeMapperTest {
 
   @Test
   public void testMergedValuesWithConcatenatedSelects() throws Exception {
-    Rule rule = createRule("x", "myrule",
+    Rule rule = scratchRule("x", "myrule",
         "sh_binary(",
         "    name = 'myrule',",
         "    srcs = select({",