Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,29 @@ public static Predicate<Target> tagFilter(List<String> tagFilterList) {
};
}

/**
* Returns a predicate to be used for test rule name filtering, i.e., that only accepts tests that match
* a required rule name and not an excluded rule name.
*/
public static Predicate<Target> ruleFilter(List<String> ruleFilterList) {
Pair<Collection<String>, Collection<String>> ruleLists =
TestTargetUtils.sortTagsBySense(ruleFilterList);
final Collection<String> requiredRules = ruleLists.first;
final Collection<String> excludedRules = ruleLists.second;
Comment on lines +389 to +392

Choose a reason for hiding this comment

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

medium

TestTargetUtils.sortTagsBySense is designed for parsing test tags and includes special logic for + prefixes and the "manual" tag. This logic is not applicable to rule names. Using it here could lead to unexpected behavior if a rule name happened to be "manual" or start with "+".

It would be more correct and robust to use a simple parser that only handles the - prefix for exclusions. A similar parser is implemented in TestFilter.testRuleFilter in this same PR. To centralize this logic, you could implement the parsing here and have TestFilter reuse it.

    final java.util.Set<String> requiredRules = new java.util.HashSet<>();
    final java.util.Set<String> excludedRules = new java.util.HashSet<>();
    for (String ruleFilter : ruleFilterList) {
      if (ruleFilter.startsWith("-")) {
        excludedRules.add(ruleFilter.substring(1));
      } else {
        requiredRules.add(ruleFilter);
      }
    }

return input -> {
if (requiredRules.isEmpty() && excludedRules.isEmpty()) {
return true;
}

if (!(input instanceof Rule)) {
return requiredRules.isEmpty();
}

return TestTargetUtils.testMatchesRuleFilters(
((Rule) input).getRuleClass(), requiredRules, excludedRules);
};
}

/** Return {@link Location} for {@link Target} target, if it should not be null. */
@Nullable
public static Location getLocationMaybe(Target target) {
Comment on lines 381 to 409
Copy link

Choose a reason for hiding this comment

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

Inefficient Filter Evaluation in Hot Code Path

The current implementation uses general-purpose Collection interfaces for rule filtering, which can lead to O(n) lookup operations in the filter evaluation. Since this filter will be applied to potentially thousands of targets in the build graph, the inefficient data structure choice could cause significant performance degradation. For large builds with many targets, this could increase target evaluation time by 10-20% due to repeated linear searches through the collections.

  public static Predicate<Target> ruleFilter(List<String> ruleFilterList) {
    Pair<Collection<String>, Collection<String>> ruleLists =
        TestTargetUtils.sortTagsBySense(ruleFilterList);
    final Set<String> requiredRules = new HashSet<>(ruleLists.first);
    final Set<String> excludedRules = new HashSet<>(ruleLists.second);
    return input -> {
      if (requiredRules.isEmpty() && excludedRules.isEmpty()) {
        return true;
      }

      if (!(input instanceof Rule)) {
        return requiredRules.isEmpty();
      }

      return TestTargetUtils.testMatchesRuleFilters(
          ((Rule) input).getRuleClass(), requiredRules, excludedRules);
    };

References

Standard: Google's Core Performance Principles - Data Structure Selection for Hot Code Paths

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,18 @@ private static boolean testMatchesFilters(
return testMatchesFilters(testTags, requiredTags, excludedTags);
}

/**
* Returns whether a test with a rule name matches a filter (as specified by the set
* of its positive and its negative filters).
*/
public static boolean testMatchesRuleFilters(
String ruleName,
Collection<String> requiredRules,
Collection<String> excludedRules) {
return (requiredRules.isEmpty() || requiredRules.contains(ruleName)) &&
!excludedRules.contains(ruleName);
}

Comment on lines 91 to +105
Copy link

Choose a reason for hiding this comment

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

Inefficient Rule Filter Implementation in TestTargetUtils

The testMatchesRuleFilters method accepts Collection interfaces rather than Set interfaces, which doesn't guarantee O(1) lookup performance. Since this method will be called for every target during filtering, using a potentially inefficient data structure could cause performance degradation. The implementation also doesn't optimize for the common case where excludedRules is checked first, which could short-circuit evaluation and improve performance when there are many exclusions.

  public static boolean testMatchesRuleFilters(
      String ruleName,
      Collection<String> requiredRules,
      Collection<String> excludedRules) {
    // Check exclusions first for short-circuit evaluation
    if (excludedRules.contains(ruleName)) {
      return false;
    }
    return requiredRules.isEmpty() || requiredRules.contains(ruleName);
  }

References

Standard: Google's Performance Best Practices - Predicate Evaluation Optimization

/**
* Filters 'tests' (by mutation) according to the 'tags' attribute, specifically those that
* match ALL of the tags in tagsAttribute.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,35 @@ public class LoadingOptions extends OptionsBase {
)
public List<String> testLangFilterList;

@Option(
name = "build_rule_filters",
converter = CommaSeparatedOptionListConverter.class,
defaultValue = "",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
help =
"Specifies a comma-separated list of rule names. Each rule name can be optionally "
+ "preceded with '-' to specify excluded rule names. Only those targets will be built that "
+ "equal the positive rule name or do not equal the negative rule name. This option "
+ "does not affect the set of tests executed with the 'test' command; those are be "
+ "governed by the test filtering options, for example '--test_rule_filters'"
)
public List<String> buildRuleFilterList;

@Option(
name = "test_rule_filters",
converter = CommaSeparatedOptionListConverter.class,
defaultValue = "",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
help =
"Specifies a comma-separated list of rule names. Each rule name can be optionally "
+ "preceded with '-' to specify excluded rule names. Only those test targets will be "
+ "found that equal the positive rule name or do not equal the negative rule name."
+ "This option affects --build_tests_only behavior and the test command."
)
public List<String> testRuleFilterList;

@Option(
name = "build_manual_tests",
defaultValue = "false",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,25 +46,29 @@ public static TestFilter forOptions(LoadingOptions options) {
ImmutableSet.copyOf(options.testSizeFilterSet),
ImmutableSet.copyOf(options.testTimeoutFilterSet),
ImmutableList.copyOf(options.testTagFilterList),
ImmutableList.copyOf(options.testLangFilterList));
ImmutableList.copyOf(options.testLangFilterList),
ImmutableList.copyOf(options.testRuleFilterList));
}

private final ImmutableSet<TestSize> testSizeFilterSet;
private final ImmutableSet<TestTimeout> testTimeoutFilterSet;
private final ImmutableList<String> testTagFilterList;
private final ImmutableList<String> testLangFilterList;
private final ImmutableList<String> testRuleFilterList;
private final Predicate<Target> impl;

@VisibleForSerialization
TestFilter(
ImmutableSet<TestSize> testSizeFilterSet,
ImmutableSet<TestTimeout> testTimeoutFilterSet,
ImmutableList<String> testTagFilterList,
ImmutableList<String> testLangFilterList) {
ImmutableList<String> testLangFilterList,
ImmutableList<String> testRuleFilterList) {
this.testSizeFilterSet = testSizeFilterSet;
this.testTimeoutFilterSet = testTimeoutFilterSet;
this.testTagFilterList = testTagFilterList;
this.testLangFilterList = testLangFilterList;
this.testRuleFilterList = testRuleFilterList;
Predicate<Target> testFilter = ALWAYS_TRUE;
if (!testSizeFilterSet.isEmpty()) {
testFilter = testFilter.and(testSizeFilter(testSizeFilterSet));
Expand All @@ -78,6 +82,9 @@ public static TestFilter forOptions(LoadingOptions options) {
if (!testLangFilterList.isEmpty()) {
testFilter = testFilter.and(testLangFilter(testLangFilterList));
}
if (!testRuleFilterList.isEmpty()) {
testFilter = testFilter.and(testRuleFilter(testRuleFilterList));
}
Comment on lines +85 to +87

Choose a reason for hiding this comment

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

medium

There's an opportunity to reduce code duplication here. The testRuleFilter method (lines 172-194) duplicates the logic for rule filtering that is also being introduced in TargetUtils.ruleFilter.

To improve maintainability, you can replace this with a direct call to TargetUtils.ruleFilter. This also makes the test filtering more robust, as TargetUtils.ruleFilter correctly handles non-Rule targets.

After this change, the private testRuleFilter method in this file will be unused and can be removed.

Suggested change
if (!testRuleFilterList.isEmpty()) {
testFilter = testFilter.and(testRuleFilter(testRuleFilterList));
}
if (!testRuleFilterList.isEmpty()) {
testFilter = testFilter.and(TargetUtils.ruleFilter(testRuleFilterList));
}

impl = testFilter;
}

Expand All @@ -89,7 +96,7 @@ public boolean apply(@Nullable Target input) {
@Override
public int hashCode() {
return Objects.hash(testSizeFilterSet, testTimeoutFilterSet, testTagFilterList,
testLangFilterList);
testLangFilterList, testRuleFilterList);
}

@Override
Expand All @@ -103,7 +110,8 @@ public boolean equals(Object o) {
return f.testSizeFilterSet.equals(testSizeFilterSet)
&& f.testTimeoutFilterSet.equals(testTimeoutFilterSet)
&& f.testTagFilterList.equals(testTagFilterList)
&& f.testLangFilterList.equals(testLangFilterList);
&& f.testLangFilterList.equals(testLangFilterList)
&& f.testRuleFilterList.equals(testRuleFilterList);
}

@Override
Expand All @@ -113,6 +121,7 @@ public String toString() {
.add("testTimeoutFilterSet", testTimeoutFilterSet)
.add("testTagFilterList", testTagFilterList)
.add("testLangFilterList", testLangFilterList)
.add("testRuleFilterList", testRuleFilterList)
.toString();
}

Expand Down Expand Up @@ -159,4 +168,28 @@ private static Predicate<Target> testLangFilter(List<String> langFilterList) {
&& !excludedLangs.contains(ruleLang);
};
}

/**
* Returns a predicate to be used for test rule filtering, i.e., that only accepts tests of
* the specified rule names.
*/
private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) {
final Set<String> requiredRules = new HashSet<>();
final Set<String> excludedRules = new HashSet<>();

for (String ruleFilter : ruleFilterList) {
if (ruleFilter.startsWith("-")) {
ruleFilter = ruleFilter.substring(1);
excludedRules.add(ruleFilter);
} else {
requiredRules.add(ruleFilter);
}
}

return rule -> {
String ruleName = rule.getRuleClass();
return (requiredRules.isEmpty() || requiredRules.contains(ruleName))
&& !excludedRules.contains(ruleName);
Comment on lines 168 to +192
Copy link

Choose a reason for hiding this comment

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

Inconsistent Filter Implementation in TestFilter.java

The implementation of testRuleFilter in TestFilter.java manually parses the rule filter list instead of using the existing TestTargetUtils.sortTagsBySense method that's used in the new ruleFilter method in TargetUtils.java. This inconsistency creates two different ways of parsing the same type of filter list in the codebase, which violates the principle of consistency and could lead to maintenance issues if the parsing logic needs to change.

    /**
   * Returns a predicate to be used for test rule filtering, i.e., that only accepts tests of
   * the specified rule names.
   */
  private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) {
    Pair<Collection<String>, Collection<String>> ruleLists =
        TestTargetUtils.sortTagsBySense(ruleFilterList);
    final Set<String> requiredRules = new HashSet<>(ruleLists.first);
    final Set<String> excludedRules = new HashSet<>(ruleLists.second);

References

Standard: DRY Principle, Clean Code - Chapter 10: Classes, Principle of Consistency

};
}
Comment on lines +172 to +194
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add type safety check for Rule targets.

The testRuleFilter method calls rule.getRuleClass() without verifying that the target is actually a Rule instance, which could cause a ClassCastException.

  private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) {
    final Set<String> requiredRules = new HashSet<>();
    final Set<String> excludedRules = new HashSet<>();

    for (String ruleFilter : ruleFilterList) {
      if (ruleFilter.startsWith("-")) {
        ruleFilter = ruleFilter.substring(1);
        excludedRules.add(ruleFilter);
      } else {
        requiredRules.add(ruleFilter);
      }
    }

-   return rule -> {
+   return target -> {
+     if (!(target instanceof Rule rule)) {
+       return false;
+     }
      String ruleName = rule.getRuleClass();
      return (requiredRules.isEmpty() || requiredRules.contains(ruleName))
          && !excludedRules.contains(ruleName);
    };
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Returns a predicate to be used for test rule filtering, i.e., that only accepts tests of
* the specified rule names.
*/
private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) {
final Set<String> requiredRules = new HashSet<>();
final Set<String> excludedRules = new HashSet<>();
for (String ruleFilter : ruleFilterList) {
if (ruleFilter.startsWith("-")) {
ruleFilter = ruleFilter.substring(1);
excludedRules.add(ruleFilter);
} else {
requiredRules.add(ruleFilter);
}
}
return rule -> {
String ruleName = rule.getRuleClass();
return (requiredRules.isEmpty() || requiredRules.contains(ruleName))
&& !excludedRules.contains(ruleName);
};
}
/**
* Returns a predicate to be used for test rule filtering, i.e., that only accepts tests of
* the specified rule names.
*/
private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) {
final Set<String> requiredRules = new HashSet<>();
final Set<String> excludedRules = new HashSet<>();
for (String ruleFilter : ruleFilterList) {
if (ruleFilter.startsWith("-")) {
ruleFilter = ruleFilter.substring(1);
excludedRules.add(ruleFilter);
} else {
requiredRules.add(ruleFilter);
}
}
return target -> {
if (!(target instanceof Rule rule)) {
return false;
}
String ruleName = rule.getRuleClass();
return (requiredRules.isEmpty() || requiredRules.contains(ruleName))
&& !excludedRules.contains(ruleName);
};
}
🤖 Prompt for AI Agents
In src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java between
lines 172 and 194, the method testRuleFilter calls rule.getRuleClass() without
checking if the target is an instance of Rule, risking a ClassCastException. To
fix this, add a type check to verify that the target is a Rule before calling
getRuleClass(), and handle cases where it is not a Rule appropriately, such as
returning false or skipping those targets in the predicate.

}
Original file line number Diff line number Diff line change
Expand Up @@ -3002,6 +3002,7 @@ public TargetPatternPhaseValue loadTargetPatternsWithFilters(
options.buildTestsOnly,
determineTests,
ImmutableList.copyOf(options.buildTagFilterList),
ImmutableList.copyOf(options.buildRuleFilterList),
options.buildManualTests,
options.expandTestSuites,
TestFilter.forOptions(options));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,10 @@ private static ResolvedTargets<Target> mergeAll(
}
}

ResolvedTargets<Target> result =
builder.filter(TargetUtils.tagFilter(options.getBuildTargetFilter())).build();
builder.filter(TargetUtils.tagFilter(options.getBuildTargetFilter()));
builder.filter(TargetUtils.ruleFilter(options.getBuildRuleFilter()));

ResolvedTargets<Target> result = builder.build();
if (options.getCompileOneDependency()) {
EnvironmentBackedRecursivePackageProvider environmentBackedRecursivePackageProvider =
new EnvironmentBackedRecursivePackageProvider(env);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ public static TargetPatternPhaseKey key(
boolean buildTestsOnly,
boolean determineTests,
ImmutableList<String> buildTargetFilter,
ImmutableList<String> buildRuleFilter,
boolean buildManualTests,
boolean expandTestSuites,
@Nullable TestFilter testFilter) {
Expand All @@ -165,6 +166,7 @@ public static TargetPatternPhaseKey key(
buildTestsOnly,
determineTests,
buildTargetFilter,
buildRuleFilter,
buildManualTests,
expandTestSuites,
testFilter);
Expand All @@ -181,7 +183,7 @@ public static TargetPatternPhaseKey key(
public static SkyKey keyWithoutFilters(
ImmutableList<String> targetPatterns, PathFragment offset) {
return new TargetPatternPhaseKey(
targetPatterns, offset, false, false, false, ImmutableList.of(), false, false, null);
targetPatterns, offset, false, false, false, ImmutableList.of(), ImmutableList.of(), false, false, null);
}

/** The configuration needed to run the target pattern evaluation phase. */
Expand All @@ -194,6 +196,7 @@ static final class TargetPatternPhaseKey implements SkyKey {
private final boolean buildTestsOnly;
private final boolean determineTests;
private final ImmutableList<String> buildTargetFilter;
private final ImmutableList<String> buildRuleFilter;
private final boolean buildManualTests;
private final boolean expandTestSuites;
@Nullable private final TestFilter testFilter;
Expand All @@ -205,6 +208,7 @@ private TargetPatternPhaseKey(
boolean buildTestsOnly,
boolean determineTests,
ImmutableList<String> buildTargetFilter,
ImmutableList<String> buildRuleFilter,
boolean buildManualTests,
boolean expandTestSuites,
@Nullable TestFilter testFilter) {
Expand All @@ -214,6 +218,7 @@ private TargetPatternPhaseKey(
this.buildTestsOnly = buildTestsOnly;
this.determineTests = determineTests;
this.buildTargetFilter = Preconditions.checkNotNull(buildTargetFilter);
this.buildRuleFilter = Preconditions.checkNotNull(buildRuleFilter);
this.buildManualTests = buildManualTests;
this.expandTestSuites = expandTestSuites;
this.testFilter = testFilter;
Expand Down Expand Up @@ -251,6 +256,10 @@ public ImmutableList<String> getBuildTargetFilter() {
return buildTargetFilter;
}

public ImmutableList<String> getBuildRuleFilter() {
return buildRuleFilter;
}

public boolean getBuildManualTests() {
return buildManualTests;
}
Expand Down Expand Up @@ -305,6 +314,7 @@ public boolean equals(Object obj) {
&& other.buildTestsOnly == buildTestsOnly
&& other.determineTests == determineTests
&& other.buildTargetFilter.equals(buildTargetFilter)
&& other.buildRuleFilter.equals(buildRuleFilter)
&& other.buildManualTests == buildManualTests
&& other.expandTestSuites == expandTestSuites
&& Objects.equals(other.testFilter, testFilter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,48 @@ public void testFilterByTag() throws Exception {
assertThat(tagFilter.apply(tag1b)).isFalse();
}

@Test
public void testFilterByRuleName() throws Exception {
scratch.file(
"tests/BUILD",
"""
load("//test_defs:foo_binary.bzl", "foo_binary")
load("//test_defs:foo_test.bzl", "foo_test")
foo_binary(
name = "foo_binary",
)

foo_test(
name = "foo_test",
)
""");

Target fooBinary = getTarget("//tests:foo_binary");
Target fooTest = getTarget("//tests:foo_test");

Predicate<Target> ruleFilter = TargetUtils.ruleFilter(Lists.<String>newArrayList());
assertThat(ruleFilter.apply(fooBinary)).isTrue();
assertThat(ruleFilter.apply(fooTest)).isTrue();
ruleFilter = TargetUtils.ruleFilter(Lists.newArrayList("foo_binary", "foo_test"));
assertThat(ruleFilter.apply(fooBinary)).isTrue();
assertThat(ruleFilter.apply(fooTest)).isTrue();
ruleFilter = TargetUtils.ruleFilter(Lists.newArrayList("foo_binary"));
assertThat(ruleFilter.apply(fooBinary)).isTrue();
assertThat(ruleFilter.apply(fooTest)).isFalse();
ruleFilter = TargetUtils.ruleFilter(Lists.newArrayList("-foo_test"));
assertThat(ruleFilter.apply(fooBinary)).isTrue();
assertThat(ruleFilter.apply(fooTest)).isFalse();
// Applying same tag as positive and negative filter produces an empty
// result because the negative filter is applied first and positive filter will
// not match anything.
ruleFilter = TargetUtils.ruleFilter(Lists.newArrayList("foo_test", "-foo_test"));
assertThat(ruleFilter.apply(fooBinary)).isFalse();
assertThat(ruleFilter.apply(fooTest)).isFalse();
ruleFilter = TargetUtils.ruleFilter(Lists.newArrayList("foo_test", "-foo_binary"));
assertThat(ruleFilter.apply(fooBinary)).isFalse();
assertThat(ruleFilter.apply(fooTest)).isTrue();
}

@Test
public void testExecutionInfo() throws Exception {
scratch.file(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,34 @@ public void testFilterBySize() {
public void testFilterByLang() {
LoadingOptions options = new LoadingOptions();
options.testLangFilterList = ImmutableList.of("positive", "-negative");
options.testRuleFilterList = ImmutableList.of();
options.testSizeFilterSet = ImmutableSet.of();
options.testTimeoutFilterSet = ImmutableSet.of();
options.testTagFilterList = ImmutableList.of();
TestFilter filter = TestFilter.forOptions(options);
Package pkg = mock(Package.class);
RuleClass ruleClass = mock(RuleClass.class);
when(ruleClass.getDefaultImplicitOutputsFunction())
.thenReturn(SafeImplicitOutputsFunction.NONE);
when(ruleClass.getAttributeProvider()).thenReturn(mock(AttributeProvider.class));
Rule mockRule =
new Rule(
pkg,
Label.parseCanonicalUnchecked("//pkg:a"),
ruleClass,
Location.fromFile(""),
/* interiorCallStack= */ null);
when(ruleClass.getName()).thenReturn("positive_test");
assertThat(filter.apply(mockRule)).isTrue();
when(ruleClass.getName()).thenReturn("negative_test");
assertThat(filter.apply(mockRule)).isFalse();
}

@Test
public void testFilterByRule() {
LoadingOptions options = new LoadingOptions();
options.testLangFilterList = ImmutableList.of();
options.testRuleFilterList = ImmutableList.of("positive_test", "-negative_test");
options.testSizeFilterSet = ImmutableSet.of();
options.testTimeoutFilterSet = ImmutableSet.of();
options.testTagFilterList = ImmutableList.of();
Expand Down
Loading
Loading