Introduce flags to allow filtering of targets by rule name #27
Conversation
This commit introduces two new flags: `--build_rule_filters` & `--test_rule_filters`. They behave in a very similar manner to existing filter flags, e.g. tag, timeout, size, lang, etc., and will allow users to filter out or in targets for building/testing that use a specific rule.
WalkthroughThis change introduces rule name-based filtering for build and test targets throughout the codebase. New command-line options allow users to specify rule name filters for building and testing. Supporting logic, predicates, and test cases are added to apply these filters in target selection, test filtering, and key generation for target pattern evaluation, with associated updates to constructors, methods, and equality checks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoadingOptions
participant SkyframeExecutor
participant TargetPatternPhaseValue
participant TargetPatternPhaseFunction
participant TargetUtils
participant TestFilter
User->>LoadingOptions: Set build_rule_filters / test_rule_filters
User->>SkyframeExecutor: Build or test command
SkyframeExecutor->>TargetPatternPhaseValue: key(..., buildRuleFilterList)
SkyframeExecutor->>TargetPatternPhaseFunction: mergeAll(...)
TargetPatternPhaseFunction->>TargetUtils: ruleFilter(buildRuleFilterList)
TargetPatternPhaseFunction->>TargetUtils: tagFilter(...)
TargetPatternPhaseFunction->>Target: Apply filters
SkyframeExecutor->>TestFilter: forOptions(LoadingOptions)
TestFilter->>Target: testRuleFilter(testRuleFilterList)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.javaNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces new filtering capabilities to the build system, allowing users to select or exclude targets based on their rule names. This enhancement provides more granular control over which targets are processed during build and test commands, complementing existing filtering options like tags and sizes.
Highlights
- New Command-Line Flags: Two new flags,
--build_rule_filtersand--test_rule_filters, have been added to enable filtering of build and test targets, respectively, by their rule names. These flags support both inclusion and exclusion patterns. - Core Rule Filtering Logic: New utility methods have been implemented in
TargetUtilsandTestTargetUtilsto create predicates that can filterTargetobjects based on their associated rule names, supporting positive and negative matches. - Integration into Target Loading: The new rule filtering mechanism is integrated into the target pattern loading phase, ensuring that only targets matching the specified rule filters are considered for building or testing.
- Test Filter Extension: The
TestFilterclass has been extended to incorporate the new--test_rule_filtersoption, allowing test commands to leverage rule-based filtering. - Comprehensive Unit Tests: New unit tests have been added for
TargetUtilsandTestTargetUtilsto validate the correctness and behavior of the new rule filtering predicates, along with updates toTargetPatternPhaseKeyTestfor compatibility.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
There was a problem hiding this comment.
Code Review
This pull request introduces new flags --build_rule_filters and --test_rule_filters to allow filtering targets by their rule names. The implementation is sound and includes tests for the new functionality. My review focuses on improving maintainability by reducing code duplication between the filtering logic for build and test rules, and on correcting the use of a utility method to ensure it's appropriate for its new context.
| Pair<Collection<String>, Collection<String>> ruleLists = | ||
| TestTargetUtils.sortTagsBySense(ruleFilterList); | ||
| final Collection<String> requiredRules = ruleLists.first; | ||
| final Collection<String> excludedRules = ruleLists.second; |
There was a problem hiding this comment.
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);
}
}| if (!testRuleFilterList.isEmpty()) { | ||
| testFilter = testFilter.and(testRuleFilter(testRuleFilterList)); | ||
| } |
There was a problem hiding this comment.
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.
| if (!testRuleFilterList.isEmpty()) { | |
| testFilter = testFilter.and(testRuleFilter(testRuleFilterList)); | |
| } | |
| if (!testRuleFilterList.isEmpty()) { | |
| testFilter = testFilter.and(TargetUtils.ruleFilter(testRuleFilterList)); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (1)
291-301: Critical: Missing buildRuleFilter in hashCode calculation.The
hashCode()method doesn't include the newbuildRuleFilterfield, which violates the equals-hashCode contract and could cause incorrect behavior in hash-based collections.@Override public int hashCode() { return Objects.hash( targetPatterns, offset, compileOneDependency, buildTestsOnly, determineTests, + buildTargetFilter, + buildRuleFilter, buildManualTests, expandTestSuites, testFilter); }
🧹 Nitpick comments (3)
src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java (1)
130-132: Fix comment to reference "rule" instead of "tag".The comment mentions "tag" but should reference "rule" since this test is for rule filtering, not tag filtering.
- // Applying same tag as positive and negative filter produces an empty + // Applying same rule as positive and negative filter produces an empty // result because the negative filter is applied first and positive filter will // not match anything.src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java (1)
141-142: Fix grammar in help text.There's a grammatical error in the help text: "those are be governed" should be "those are governed".
- + "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'" + + "does not affect the set of tests executed with the 'test' command; those are " + + "governed by the test filtering options, for example '--test_rule_filters'"src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java (1)
144-169: Extract common mock setup to reduce duplication.The mock setup code is identical between
testFilterByLang()andtestFilterByRule(). Consider extracting the common mock creation into a helper method to improve maintainability.+ private Rule createMockRule(String ruleClassName) { + Package pkg = mock(Package.class); + RuleClass ruleClass = mock(RuleClass.class); + when(ruleClass.getDefaultImplicitOutputsFunction()) + .thenReturn(SafeImplicitOutputsFunction.NONE); + when(ruleClass.getAttributeProvider()).thenReturn(mock(AttributeProvider.class)); + when(ruleClass.getName()).thenReturn(ruleClassName); + return new Rule( + pkg, + Label.parseCanonicalUnchecked("//pkg:a"), + ruleClass, + Location.fromFile(""), + /* interiorCallStack= */ null); + } @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(); 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(); + Rule mockRule = createMockRule("positive_test"); + assertThat(filter.apply(mockRule)).isTrue(); + mockRule = createMockRule("negative_test"); + assertThat(filter.apply(mockRule)).isFalse(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java(1 hunks)src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java(1 hunks)src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java(1 hunks)src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java(6 hunks)src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java(1 hunks)src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java(1 hunks)src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java(8 hunks)src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java(1 hunks)src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java(1 hunks)src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java (1)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
TargetUtils(44-455)
src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java (1)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
TargetUtils(44-455)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java (1)
TestTargetUtils(27-151)
src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java (1)
src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java (1)
SafeImplicitOutputsFunction(166-181)
🔇 Additional comments (17)
src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java (1)
3005-3005: LGTM! Correctly integrates build rule filtering into target pattern loading.The addition of
ImmutableList.copyOf(options.buildRuleFilterList)as a parameter toTargetPatternPhaseValue.keyfollows the established pattern used for other filter lists (likebuildTagFilterListon the previous line). This change enables rule name-based filtering during the target pattern resolution phase, which aligns with the PR objective of introducing flags to filter targets by rule name.src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java (1)
370-372: LGTM! Rule filtering integration is well-placed.The addition of rule filtering alongside tag filtering follows the established pattern and maintains consistency with the existing filtering approach. The placement ensures filters are applied at the correct stage in the target resolution process.
src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java (1)
94-104: LGTM! Rule filtering logic is correct and well-documented.The implementation correctly handles both inclusion and exclusion filtering with clear, concise logic. The method signature and documentation are appropriate for this utility function.
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
384-405: LGTM! Rule filter implementation follows established patterns.The
ruleFiltermethod correctly mirrors the structure and behavior of the existingtagFiltermethod. The implementation properly handles both Rule and non-Rule targets, and appropriately delegates toTestTargetUtils.testMatchesRuleFiltersfor the core filtering logic.src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java (1)
118-142: LGTM! Well-structured test with proper isolation.The test properly isolates language filtering by setting other filter lists to empty and uses appropriate mocking. The assertions correctly verify both positive and negative filtering behavior.
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (5)
158-173: LGTM! Factory method properly updated.The factory method correctly incorporates the new
buildRuleFilterparameter and passes it through to the constructor.
185-187: LGTM! Default value correctly provided.The
keyWithoutFiltersmethod appropriately provides an empty list for the newbuildRuleFilterparameter, maintaining backward compatibility.
199-221: LGTM! Field properly integrated.The new
buildRuleFilterfield is correctly added with proper null checking and initialization.
259-261: LGTM! Getter method properly implemented.The getter follows the established pattern and provides access to the new field.
317-317: LGTM! Equals method properly updated.The
equals()method correctly includes comparison of the newbuildRuleFilterfield.src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java (4)
49-50: LGTM! Factory method properly updated.The
forOptionsmethod correctly incorporates the newtestRuleFilterListparameter.
57-87: LGTM! Field properly integrated into constructor and filtering logic.The new
testRuleFilterListfield is correctly added to the constructor and integrated into the filtering pipeline following the established pattern.
99-114: LGTM! Object contract methods properly updated.The
hashCode()andequals()methods correctly include the newtestRuleFilterListfield, maintaining the object contract.
124-124: LGTM! toString method updated.The debug representation correctly includes the new field.
src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java (3)
56-136: LGTM! Test cases systematically updated.All equality test cases have been properly updated to include the new
buildRuleFilterparameter, maintaining comprehensive coverage of the key's equality behavior.
156-176: LGTM! Helper method properly updated.The
ofhelper method correctly incorporates the newbuildRuleFilterparameter and passes it through to the factory method.
178-181: LGTM! Simple overload provides appropriate default.The simplified
ofmethod correctly provides an empty list as the default value forbuildRuleFilter.
| /** | ||
| * 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); | ||
| }; | ||
| } |
There was a problem hiding this comment.
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.
| /** | |
| * 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.
Solid Implementation - Let's Address Consistency & Performance Issues!
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| }; | ||
| } | ||
|
|
||
| /** | ||
| * 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; | ||
| 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) { |
There was a problem hiding this comment.
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
| 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
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
| && !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); |
There was a problem hiding this comment.
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
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Summary by CodeRabbit
New Features
Bug Fixes
Tests