Skip to content

Provide --experimental_instrumentation_filter_fragment cli option #15

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Provide --experimental_instrumentation_filter_fragment cli option #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 18 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.analysis.config;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -44,6 +45,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.common.options.TriState;
import com.google.devtools.common.options.OptionsParsingException;
import java.io.PrintStream;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -143,6 +145,8 @@ public void reportInvalidOptions(EventHandler reporter) {
}
}

private final RegexFilter instrumentationFilter;

/**
* Compute the test environment, which, at configuration level, is a pair consisting of the
* statically set environment variables with their values and the set of environment variables to
Expand Down Expand Up @@ -298,6 +302,19 @@ private static ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfig
this.reservedActionMnemonics = reservedActionMnemonics;
this.commandLineLimits = new CommandLineLimits(options.minParamFileSize);
this.defaultFeatures = FeatureSet.parse(options.defaultFeatures);

if (!options.instrumentationFilterFragment.isEmpty()) {
// A regex-based instrumentation filter instance is formed by concatenating
// string values of --experimental_instrumentation_filter_fragment.
try {
this.instrumentationFilter = new RegexFilter.RegexFilterConverter().convert(
Joiner.on(",").join(options.instrumentationFilterFragment));
} catch (OptionsParsingException e) {
throw new RuntimeException(e);
}
} else {
this.instrumentationFilter = options.instrumentationFilter;
}
Comment on lines +306 to +317
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Implementation handles the new option properly but could improve error handling

The implementation correctly joins multiple filter fragments and creates a RegexFilter when the experimental option is specified, falling back to the legacy filter otherwise.

Consider improving the error handling by including a more descriptive error message when wrapping the OptionsParsingException:

try {
  this.instrumentationFilter = new RegexFilter.RegexFilterConverter().convert(
    Joiner.on(",").join(options.instrumentationFilterFragment));
} catch (OptionsParsingException e) {
-  throw new RuntimeException(e);
+  throw new RuntimeException("Failed to parse experimental_instrumentation_filter_fragment: " + e.getMessage(), e);
}

This would make debugging easier by providing context about which option caused the error.

}

@Override
Expand Down Expand Up @@ -566,7 +583,7 @@ public Iterable<String> getVariableShellEnvironment() {
* identify targets to be instrumented in the coverage mode.
*/
public RegexFilter getInstrumentationFilter() {
return options.instrumentationFilter;
return instrumentationFilter;
}

/**
Expand Down
17 changes: 17 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelListConverter;
Expand Down Expand Up @@ -274,6 +275,22 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
+ "instrumented unless --instrument_test_targets is enabled.")
public RegexFilter instrumentationFilter;

@Option(
name = "experimental_instrumentation_filter_fragment",
allowMultiple = true,
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
help =
"When coverage is enabled, only rules with names included by the "
+ "specified regex-based filter will be instrumented. Rules prefixed "
+ "with '-' are excluded instead. Note that only non-test rules are "
+ "instrumented unless --instrument_test_targets is enabled. "
+ "Excluded filters always override included ones. This option can be used "
+ "multiple times. If this option is provided --instrumentation_filter "
+ "has no effect.")
public List<String> instrumentationFilterFragment;

@Option(
name = "instrument_test_targets",
defaultValue = "false",
Expand Down
74 changes: 74 additions & 0 deletions src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3020,6 +3020,80 @@ private void setUpCoverageInstrumentedTest() throws Exception {
""");
}

@Test
public void testCoverageExperimentalInstrumentedFragmentCoverageDisabled() throws Exception {
setUpCoverageInstrumentedTest();
useConfiguration(
"--nocollect_code_coverage",
"--experimental_instrumentation_filter_fragment=.");
StarlarkRuleContext ruleContext = createRuleContext("//test:foo");
setRuleContext(ruleContext);
Object result = ev.eval("ruleContext.coverage_instrumented()");
assertThat((Boolean) result).isFalse();
}

@Test
public void testCoverageExperimentalInstrumentedFragmentFalseForSourceFileLabel() throws Exception {
setUpCoverageInstrumentedTest();
useConfiguration(
"--collect_code_coverage",
"--experimental_instrumentation_filter_fragment=:foo",
"--experimental_instrumentation_filter_fragment=:bar");
setRuleContext(createRuleContext("//test:foo"));
Object result = ev.eval("ruleContext.coverage_instrumented(ruleContext.attr.srcs[0])");
assertThat((Boolean) result).isFalse();
}

@Test
public void testCoverageExperimentalInstrumentedFragmentDoesNotMatchFilter() throws Exception {
setUpCoverageInstrumentedTest();
useConfiguration(
"--collect_code_coverage",
"--experimental_instrumentation_filter_fragment=:foo",
"--experimental_instrumentation_filter_fragment=-:bar");
setRuleContext(createRuleContext("//test:bar"));
Object result = ev.eval("ruleContext.coverage_instrumented()");
assertThat((Boolean) result).isFalse();
}

@Test
public void testCoverageExperimentalInstrumentedFragmentMatchesFilter() throws Exception {
setUpCoverageInstrumentedTest();
useConfiguration(
"--collect_code_coverage",
"--experimental_instrumentation_filter_fragment=:foo",
"--experimental_instrumentation_filter_fragment=:bar");
setRuleContext(createRuleContext("//test:foo"));
Object result = ev.eval("ruleContext.coverage_instrumented()");
assertThat((Boolean) result).isTrue();
}

@Test
public void testCoverageExperimentalInstrumentedFragmentDoesNotMatchFilterNonDefaultLabel() throws Exception {
setUpCoverageInstrumentedTest();
useConfiguration(
"--collect_code_coverage",
"--experimental_instrumentation_filter_fragment=:foo",
// --instrumentation_filter has no effect because --experimental_instrumentation_filter_fragment is used.
"--instrumentation_filter=:bar");
setRuleContext(createRuleContext("//test:foo"));
// //test:bar does not match :foo, though //test:foo would.
Object result = ev.eval("ruleContext.coverage_instrumented(ruleContext.attr.deps[0])");
assertThat((Boolean) result).isFalse();
}

@Test
public void testCoverageExperimentalInstrumentedFragmentMatchesFilterNonDefaultLabel() throws Exception {
setUpCoverageInstrumentedTest();
useConfiguration(
"--collect_code_coverage",
"--experimental_instrumentation_filter_fragment=:bar");
setRuleContext(createRuleContext("//test:foo"));
// //test:bar does match :bar, though //test:foo would not.
Object result = ev.eval("ruleContext.coverage_instrumented(ruleContext.attr.deps[0])");
assertThat((Boolean) result).isTrue();
}

@Test
public void testCoverageInstrumentedCoverageDisabled() throws Exception {
setUpCoverageInstrumentedTest();
Expand Down
Loading