StandaloneTestStrategy: use separate mnemonic for derived spawns #28111
Conversation
|
cc: @fmeum |
github-actions
bot
added
team-Configurability
|
Work towards #28076 Ideally, I would like to give these actions their own exec group as well. But that may require a longer conversation. |
src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces distinct mnemonics for child actions spawned by TestRunnerAction, which improves the ability to identify and handle these actions specifically. The implementation is sound, using a subclass to override the mnemonic. My main feedback is to centralize the new mnemonic string constants, which are currently duplicated across four files, into the TestRunnerAction class to enhance maintainability.
| private static final String TEST_RUNNER_MNEMONIC = "TestRunner"; | ||
| private static final String TEST_XML_GENERATION_MNEMONIC = "TestXmlGeneration"; | ||
| private static final String TEST_COVERAGE_POST_PROCESSING_MNEMONIC = "TestCoveragePostProcessing"; |
There was a problem hiding this comment.
To improve maintainability and prevent potential inconsistencies, these mnemonic constants, which are duplicated across several files (CompactSpawnLogContext, StandaloneTestStrategy, SandboxStash), should be centralized.
A good place for them would be the TestRunnerAction class, alongside the existing MNEMONIC constant.
For example, in TestRunnerAction.java:
public static final String MNEMONIC = "TestRunner";
public static final String XML_GENERATION_MNEMONIC = "TestXmlGeneration";
public static final String COVERAGE_POST_PROCESSING_MNEMONIC = "TestCoveragePostProcessing";Then, other classes could reference these constants, e.g., TestRunnerAction.XML_GENERATION_MNEMONIC, which would make the code more robust and easier to maintain.
src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
Outdated
Show resolved
Hide resolved
sluongng
force-pushed
the
sluongng/testrunner-mnemonic
branch
2 times, most recently
from
85a7b76 to
8689bc3
Compare
TestRunner actions today can have child actions for distinct purposes: a. If a test run does not generate the test.xml file, we will spin up a new action to create this test.xml file. b. We will also spin up new action to handle post coverage processing when 'bazel coverage' is used. It's hard to tell these 'child' actions apart from the parent TestRunner action because they use the same action context, thus having the same target label and mnemonic. Give these child actions distinct mnemonics by cloning the parent's TestRunnerAction with the mnemonic overrided. Modify the 2 callsites for this code: 1. CompactSpawnLogContext need to tell whether it's a TestRunner action to handle the runfiles accordingly. Ensure this includes the TestCoveragePostProcessing action and NOT the TestXmlGeneration action because the latter does not need these runfiles. 2. SandboxStash can now detect these child actions based on the exact mnemonic instead of counting the outputs of TestRunner actions.
sluongng
force-pushed
the
sluongng/testrunner-mnemonic
branch
from
8689bc3 to
c42ba24
Compare
2 participants
TestRunner actions today can have child actions for distinct purposes:
a. If a test run does not generate the test.xml file, we will spin up a
new action to create this test.xml file.
b. We will also spin up new action to handle post coverage processing
when 'bazel coverage' is used.
It's hard to tell these 'child' actions apart from the parent
TestRunner action because they use the same action context, thus having
the same target label and mnemonic.
Give these child actions distinct mnemonics by cloning the parent's
TestRunnerAction with the mnemonic overrided.
Modify the 2 callsites for this code:
CompactSpawnLogContext need to tell whether it's a TestRunner action
to handle the runfiles accordingly. Ensure this includes the
TestCoveragePostProcessing action and NOT the TestXmlGeneration
action because the latter does not need these runfiles.
SandboxStash can now detect these child actions based on the exact
mnemonic instead of counting the outputs of TestRunner actions.