Skip to content

Commit

Permalink
Store the total number of runs per test
Browse files Browse the repository at this point in the history
Instead of recomputing the runs per test everywhere it's needed, compute
it once and store it in the TestTargetExecutionSettings.

This is purely a cleanup change and should have no impact on semantics.

This should not affect memory consumption, because object size is a
multiple of 8, and there were previously 11 4-byte fields (assuming
compressed oops) in this class.

PiperOrigin-RevId: 262096308
  • Loading branch information
ulfjack authored and copybara-github committed Aug 7, 2019
1 parent 5ce60ff commit 5585169
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@ private TestParams createTestAction(int shards) {
Artifact collectCoverageScript = null;
TreeMap<String, String> extraTestEnv = new TreeMap<>();

int runsPerTest =
config.getFragment(TestConfiguration.class).getRunsPerTestForLabel(ruleContext.getLabel());

TestTargetExecutionSettings executionSettings;
if (collectCodeCoverage) {
collectCoverageScript = ruleContext.getHostPrerequisiteArtifact("$collect_coverage_script");
Expand Down Expand Up @@ -302,16 +305,23 @@ private TestParams createTestAction(int shards) {
Artifact instrumentedFileManifest =
InstrumentedFileManifestAction.getInstrumentedFileManifest(ruleContext,
instrumentedFiles.getInstrumentedFiles(), metadataFiles);
executionSettings = new TestTargetExecutionSettings(ruleContext, runfilesSupport,
executable, instrumentedFileManifest, shards);
executionSettings =
new TestTargetExecutionSettings(
ruleContext,
runfilesSupport,
executable,
instrumentedFileManifest,
shards,
runsPerTest);
inputsBuilder.add(instrumentedFileManifest);
// TODO(ulfjack): Is this even ever set? If yes, does this cost us a lot of memory?
for (Pair<String, String> coverageEnvEntry : instrumentedFiles.getCoverageEnvironment()) {
extraTestEnv.put(coverageEnvEntry.getFirst(), coverageEnvEntry.getSecond());
}
} else {
executionSettings = new TestTargetExecutionSettings(ruleContext, runfilesSupport,
executable, null, shards);
executionSettings =
new TestTargetExecutionSettings(
ruleContext, runfilesSupport, executable, null, shards, runsPerTest);
}

extraTestEnv.putAll(extraEnv);
Expand All @@ -323,9 +333,6 @@ private TestParams createTestAction(int shards) {
}
}

int runsPerTest =
config.getFragment(TestConfiguration.class).getRunsPerTestForLabel(ruleContext.getLabel());

Iterable<Artifact> inputs = inputsBuilder.build();
int shardRuns = (shards > 0 ? shards : 1);
List<Artifact.DerivedArtifact> results =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
fp.addInt(shardNum);
fp.addInt(executionSettings.getTotalShards());
fp.addInt(runNumber);
fp.addInt(testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()));
fp.addInt(executionSettings.getTotalRuns());
fp.addInt(configuration.isCodeCoverageEnabled() ? 1 : 0);
fp.addStringMap(getExecutionInfo());
}
Expand Down Expand Up @@ -386,7 +386,7 @@ private boolean computeExecuteUnconditionallyFromTestStatus() {
testConfiguration.cacheTestResults(),
readCacheStatus(),
testProperties.isExternal(),
testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()));
executionSettings.getTotalRuns());
}

@VisibleForTesting
Expand Down Expand Up @@ -510,8 +510,7 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {

// When we run test multiple times, set different TEST_RANDOM_SEED values for each run.
// Don't override any previous setting.
if (testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()) > 1
&& !env.containsKey("TEST_RANDOM_SEED")) {
if (executionSettings.getTotalRuns() > 1 && !env.containsKey("TEST_RANDOM_SEED")) {
env.put("TEST_RANDOM_SEED", Integer.toString(getRunNumber() + 1));
}

Expand Down Expand Up @@ -581,7 +580,7 @@ public String getTestName() {
public String getTestSuffix() {
int totalShards = executionSettings.getTotalShards();
// Use a 1-based index for user friendliness.
int runsPerTest = testConfiguration.getRunsPerTestForLabel(getOwner().getLabel());
int runsPerTest = executionSettings.getTotalRuns();
if (totalShards > 1 && runsPerTest > 1) {
return String.format(
"(shard %d of %d, run %d of %d)", shardNum + 1, totalShards, runNumber + 1, runsPerTest);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public final class TestTargetExecutionSettings {
private final CommandLine testArguments;
private final String testFilter;
private final int totalShards;
private final int totalRuns;
private final RunUnder runUnder;
private final Artifact runUnderExecutable;
private final Artifact executable;
Expand All @@ -48,8 +49,13 @@ public final class TestTargetExecutionSettings {
private final Artifact runfilesInputManifest;
private final Artifact instrumentedFileManifest;

TestTargetExecutionSettings(RuleContext ruleContext, RunfilesSupport runfilesSupport,
Artifact executable, Artifact instrumentedFileManifest, int shards) {
TestTargetExecutionSettings(
RuleContext ruleContext,
RunfilesSupport runfilesSupport,
Artifact executable,
Artifact instrumentedFileManifest,
int shards,
int runs) {
Preconditions.checkArgument(TargetUtils.isTestRule(ruleContext.getRule()));
Preconditions.checkArgument(shards >= 0);
BuildConfiguration config = ruleContext.getConfiguration();
Expand All @@ -60,6 +66,7 @@ public final class TestTargetExecutionSettings {
CommandLine.concat(targetArgs, ImmutableList.copyOf(testConfig.getTestArguments()));

totalShards = shards;
totalRuns = runs;
runUnder = config.getRunUnder();
runUnderExecutable = getRunUnderExecutable(ruleContext);

Expand Down Expand Up @@ -96,6 +103,10 @@ public int getTotalShards() {
return totalShards;
}

public int getTotalRuns() {
return totalRuns;
}

public RunUnder getRunUnder() {
return runUnder;
}
Expand Down

0 comments on commit 5585169

Please sign in to comment.