refactor: extract adb instrumentation command builder from runTest#2491
refactor: extract adb instrumentation command builder from runTest#2491CristianStedile wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 refactors AdbController.java by extracting the logic for building the instrumentation shell command from runTest into a new helper method buildInstrumentationShellCommand. The feedback highlights a potential NullPointerException when collectCodeCoverage is true but coverageDataPath is null, as Guava's Joiner does not allow null values. Additionally, several comments and Javadocs introduced in Portuguese should be translated to English, and the indentation of the new methods should be adjusted to 4 spaces to match the rest of the codebase.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Opções de cobertura | ||
| if (collectCodeCoverage) { | ||
| tokens.add("-e"); | ||
| tokens.add("coverage"); | ||
| tokens.add("true"); | ||
| tokens.add("-e"); | ||
| tokens.add("coverageDataPath"); | ||
| tokens.add(coverageDataPath); | ||
| } |
There was a problem hiding this comment.
Since coverageDataPath is annotated with @Nullable, it can potentially be null. If collectCodeCoverage is true but coverageDataPath is null, adding it to tokens and then calling Joiner.on(" ").join(tokens) will throw a NullPointerException because Guava's Joiner is fail-fast on null values.
We should add a precondition check to ensure coverageDataPath is not null when collectCodeCoverage is true. Additionally, the comment has been translated to English to match the rest of the codebase.
| // Opções de cobertura | |
| if (collectCodeCoverage) { | |
| tokens.add("-e"); | |
| tokens.add("coverage"); | |
| tokens.add("true"); | |
| tokens.add("-e"); | |
| tokens.add("coverageDataPath"); | |
| tokens.add(coverageDataPath); | |
| } | |
| // Coverage options | |
| if (collectCodeCoverage) { | |
| checkArgument(coverageDataPath != null, "coverageDataPath cannot be null when collectCodeCoverage is true"); | |
| tokens.add("-e"); | |
| tokens.add("coverage"); | |
| tokens.add("true"); | |
| tokens.add("-e"); | |
| tokens.add("coverageDataPath"); | |
| tokens.add(coverageDataPath); | |
| } |
| boolean withAnimation, | ||
| Map<String, String> extraInstrumentationOptions) { | ||
|
|
||
| long testTimeout = size.getTestTimeout(TimeUnit.SECONDS); |
There was a problem hiding this comment.
The body of the runTest method (and also buildInstrumentationShellCommand) is indented with 2 spaces, whereas the rest of the file consistently uses 4 spaces for method bodies. Please update the indentation to 4 spaces to maintain consistency with the existing codebase.
| /** | ||
| * Constrói a linha de comando a ser passada para {@code adb shell} contendo a invocação completa | ||
| * do {@code am instrument}. | ||
| */ |
There was a problem hiding this comment.
The Javadoc comment is written in Portuguese, but the rest of the codebase and the PR description are in English. Please translate it to English to maintain consistency.
| /** | |
| * Constrói a linha de comando a ser passada para {@code adb shell} contendo a invocação completa | |
| * do {@code am instrument}. | |
| */ | |
| /** | |
| * Builds the command line to be passed to {@code adb shell} containing the complete invocation | |
| * of {@code am instrument}. | |
| */ |
|
|
||
| List<String> tokens = Lists.newArrayList(); | ||
|
|
||
| // Prefixo opcional para serviços básicos (ShellMain) |
There was a problem hiding this comment.
This comment is written in Portuguese. Please translate it to English to match the rest of the codebase.
| // Prefixo opcional para serviços básicos (ShellMain) | |
| // Optional prefix for basic services (ShellMain) |
| // Extras genéricos | ||
| for (Map.Entry<String, String> entry : extraInstrumentationOptions.entrySet()) { | ||
| tokens.add("-e"); | ||
| tokens.add(ShellUtils.shellEscape(entry.getKey())); | ||
| tokens.add(ShellUtils.shellEscape(entry.getValue())); | ||
| } |
There was a problem hiding this comment.
This comment is written in Portuguese. Please translate it to English to match the rest of the codebase.
| // Extras genéricos | |
| for (Map.Entry<String, String> entry : extraInstrumentationOptions.entrySet()) { | |
| tokens.add("-e"); | |
| tokens.add(ShellUtils.shellEscape(entry.getKey())); | |
| tokens.add(ShellUtils.shellEscape(entry.getValue())); | |
| } | |
| // Generic extras | |
| for (Map.Entry<String, String> entry : extraInstrumentationOptions.entrySet()) { | |
| tokens.add("-e"); | |
| tokens.add(ShellUtils.shellEscape(entry.getKey())); | |
| tokens.add(ShellUtils.shellEscape(entry.getValue())); | |
| } |
| // Animação (apenas API ≥ 10) | ||
| if (!withAnimation && device.getApiVersion() >= 10) { | ||
| tokens.add("--no_window_animation"); | ||
| } |
There was a problem hiding this comment.
This comment is written in Portuguese. Please translate it to English to match the rest of the codebase.
| // Animação (apenas API ≥ 10) | |
| if (!withAnimation && device.getApiVersion() >= 10) { | |
| tokens.add("--no_window_animation"); | |
| } | |
| // Animation (only API >= 10) | |
| if (!withAnimation && device.getApiVersion() >= 10) { | |
| tokens.add("--no_window_animation"); | |
| } |
| // Decisão sobre orquestrador | ||
| boolean useOrchestrator = | ||
| installBasicServices | ||
| && ORCHESTRATOR_ENABLED_RUNNERS.contains(instrumentation.getInstrumentationClass()); |
There was a problem hiding this comment.
This comment is written in Portuguese. Please translate it to English to match the rest of the codebase.
| // Decisão sobre orquestrador | |
| boolean useOrchestrator = | |
| installBasicServices | |
| && ORCHESTRATOR_ENABLED_RUNNERS.contains(instrumentation.getInstrumentationClass()); | |
| // Orchestrator decision | |
| boolean useOrchestrator = | |
| installBasicServices | |
| && ORCHESTRATOR_ENABLED_RUNNERS.contains(instrumentation.getInstrumentationClass()); |
| if (installBasicServices) { | ||
| tokens.add("||"); | ||
| tokens.add("true"); | ||
| } | ||
|
|
There was a problem hiding this comment.
This comment is written in Portuguese. Please translate it to English to match the rest of the codebase.
| if (installBasicServices) { | |
| tokens.add("||"); | |
| tokens.add("true"); | |
| } | |
| // If we use basic services, we hide the exit code of ShellMain | |
| if (installBasicServices) { | |
| tokens.add("||"); | |
| tokens.add("true"); | |
| } |
|
Sorry, all code in tools/ is obsolete and we are not accepting contributions here |
2 participants
Summary
Refactor the runTest method in AdbController to extract the construction of the ADB instrumentation command into a separate private method.
Motivation
The original runTest method contained over 50 lines dedicated to assembling the adb shell am instrument ... command. This logic was intermixed with the setup of output processors, error handling, and the actual subprocess call. As a result:
The method was difficult to read and maintain.
Adding new instrumentation arguments required navigating deep into the method.
The command-building logic could not be unit-tested independently.
Changes
Introduced buildInstrumentationShellCommand(...) – a new private method that takes all the instrumentation parameters (coverage, debug, HPROF, extras, animation, timeout, orchestrator usage, basic services) and returns the complete shell command string.
Simplified runTest to:
Determine the test timeout.
Call buildInstrumentationShellCommand to get the command.
Set up the processors and communicator.
Execute the command and handle errors.
The new method uses a local list of tokens joined by spaces, replicating the exact same argument order as before. Behavior is fully preserved.
All conditional logic (API level checks, orchestrator vs. direct runner, basic services prefix, etc.) is now encapsulated within the builder method.