-
Notifications
You must be signed in to change notification settings - Fork 159
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.
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
ISSUE-576: Allow complex ADB server commands execution #597
Changes from 4 commits
32984da
8b4ecbb
8ca3a8c
bbbe71c
858bd99
329a5df
3537b68
f3b688a
87390fc
4c78a17
faf482e
d6ac80d
1c609a4
64e67c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,4 +2,7 @@ package com.kaspersky.adbserver.commandtypes | |
|
||
import com.kaspersky.adbserver.common.api.Command | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also reflect all these cases in tests? I mean testing a single command, a command with arguments and a command with commands in arguments (complex commands with pipes and etc.). |
||
|
||
data class AdbCommand(override val body: String) : Command(body) | ||
data class AdbCommand( | ||
override val command: String, | ||
override val arguments: List<String> = emptyList() | ||
) : Command(command, arguments) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
package com.kaspersky.adbserver.device | ||
|
||
import com.kaspersky.adbserver.common.api.CommandResult | ||
import com.kaspersky.adbserver.commandtypes.AdbCommand | ||
import com.kaspersky.adbserver.commandtypes.CmdCommand | ||
import com.kaspersky.adbserver.common.api.CommandResult | ||
import com.kaspersky.adbserver.common.log.LoggerFactory | ||
import com.kaspersky.adbserver.common.log.logger.LogLevel | ||
import com.kaspersky.adbserver.common.log.logger.Logger | ||
|
@@ -27,10 +27,32 @@ object AdbTerminal { | |
AdbCommand(command) | ||
) ?: throw IllegalStateException("Please first of all call [connect] method to establish a connection") | ||
|
||
/** | ||
* Allows more control over how arguments are parsed. Each element in the [arguments] list | ||
* is used as is without tokenizing. | ||
* Refer to the Runtime::exec(String[] cmdarray) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide a full link to this method. |
||
* | ||
* Please first of all call [connect] method to establish a connection | ||
*/ | ||
fun executeAdb(command: String, arguments: List<String>): CommandResult = device?.fulfill( | ||
AdbCommand(command, arguments) | ||
) ?: throw IllegalStateException("Please first of all call [connect] method to establish a connection") | ||
|
||
/** | ||
* Please first of all call [connect] method to establish a connection | ||
*/ | ||
fun executeCmd(command: String): CommandResult = device?.fulfill( | ||
CmdCommand(command) | ||
) ?: throw IllegalStateException("Please first of all call [connect] method to establish a connection") | ||
|
||
/** | ||
* Allows more control over how arguments are parsed. Each element in the [arguments] list | ||
* is used as is without tokenizing. | ||
* Refer to the Runtime::exec(String[] cmdarray) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide a full link to this method. |
||
* | ||
* Please first of all call [connect] method to establish a connection | ||
*/ | ||
fun executeCmd(command: String, arguments: List<String>): CommandResult = device?.fulfill( | ||
CmdCommand(command, arguments) | ||
) ?: throw IllegalStateException("Please first of all call [connect] method to establish a connection") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,6 +190,14 @@ Now the logs looks like: | |
2020-09-10 12:24:27.427 10349-10378/com.kaspersky.kaspressample I/KASPRESSO_ADBSERVER: The result of command=AdbCommand(body=shell su 0 svc data disable) => CommandResult(status=SUCCESS, description=exitCode=0, message=, serviceInfo=The command was executed on desktop=Desktop-30548) | ||
``` | ||
|
||
## Complex commands | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description is unclear for people who don't have the task context. Another point is the existence of two methods that are similar to each other but their implementations are different in terms of tokenization and etc, is not great. This case may confuse devs. I think we should deprecate the first method with a very detailed explanation. I doubt that we will remove the first method but we'll give a clear picture for devs about which method they need to use. |
||
There're two kinds of ADB server methods signatures: `preformCmd(vararg commands: String)` and `performsCmd(command: String, arguments: List<String>)`. The first executes multiple | ||
commands one by one. The latter allows you to gain more control over the commands parsing - ADB server would execute commands "as is" without trying to split command into tokens. This allows you to execute | ||
commands with whitespaces in their arguments and use piping. Example: | ||
```kotlin | ||
adbServer.performCmd("bash", arguments = listOf("-c", "grep adb \"~/Documents/test file.txt\" > \"~/Documents/out file.txt\"")) | ||
``` | ||
|
||
## Development | ||
The source code of AdbServer is available in [adb-server](https://github.com/KasperskyLab/Kaspresso/tree/master/adb-server) module. <br> | ||
If you want to build `adbserver-desktop.jar` manually, just execute `./gradlew :adb-server:adbserver-desktop:assemble`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,20 @@ interface AdbServer { | |
*/ | ||
fun performCmd(vararg commands: String): List<String> | ||
|
||
/** | ||
* Performs shell commands blocking current thread. Allows more control over how arguments are parsed. | ||
matzuk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* Each list element is used as is. Refer to the https://docs.oracle.com/javase/8/docs/api/java/lang/ProcessBuilder.html. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please unify your comments. Somewhere you write class and method name, somewhere you provide a link. |
||
* | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
* | ||
* Required Permissions: INTERNET. | ||
* | ||
* @param commands commands to execute. | ||
* @throws AdbServerException if a result status of any command in @param commands is Failed | ||
* @return list of answers of commands' execution | ||
*/ | ||
fun performCmd(command: String, arguments: List<String>): String | ||
|
||
/** | ||
* Performs adb commands blocking current thread. | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
|
@@ -38,6 +52,20 @@ interface AdbServer { | |
*/ | ||
fun performAdb(vararg commands: String): List<String> | ||
|
||
/** | ||
* Performs adb commands blocking current thread. Allows more control over how arguments are parsed. | ||
* Each list element is used as is. Refer to the https://docs.oracle.com/javase/8/docs/api/java/lang/ProcessBuilder.html. | ||
* | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
* | ||
* Required Permissions: INTERNET. | ||
* | ||
* @param commands commands to execute. | ||
* @throws AdbServerException if a result status of any command in @param commands is Failed | ||
* @return list of answers of commands' execution | ||
*/ | ||
fun performAdb(command: String, arguments: List<String>): String | ||
|
||
/** | ||
* Performs shell commands blocking current thread. | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
|
@@ -50,6 +78,20 @@ interface AdbServer { | |
*/ | ||
fun performShell(vararg commands: String): List<String> | ||
|
||
/** | ||
* Performs shell commands blocking current thread. Allows more control over how arguments are parsed. | ||
* Each list element is used as is. Refer to the https://docs.oracle.com/javase/8/docs/api/java/lang/ProcessBuilder.html. | ||
* | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
* | ||
* Required Permissions: INTERNET. | ||
* | ||
* @param commands commands to execute. | ||
* @throws AdbServerException if a result status of any command in @param commands is Failed | ||
* @return list of answers of commands' execution | ||
*/ | ||
fun performShell(command: String, arguments: List<String>): String | ||
|
||
/** | ||
* Disconnect from AdbServer. | ||
* The method is called by Kaspresso after each test. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,58 +30,36 @@ class AdbServerImpl( | |
return AdbTerminal | ||
} | ||
|
||
/** | ||
* Executes shell commands blocking current thread. | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
* | ||
* Required Permissions: INTERNET. | ||
* | ||
* @param commands commands to execute. | ||
* @throws AdbServerException if a result status of any command in @param commands is Failed | ||
* @return list of answers of commands' execution | ||
*/ | ||
override fun performCmd(vararg commands: String): List<String> { | ||
return perform(commands) { | ||
adbTerminal.executeCmd(it) | ||
} | ||
} | ||
|
||
/** | ||
* Performs adb commands blocking current thread. | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
* | ||
* Required Permissions: INTERNET. | ||
* | ||
* @param commands commands to execute. | ||
* @throws AdbServerException if a result status of any command in @param commands is Failed | ||
* @return list of answers of commands' execution | ||
*/ | ||
override fun performAdb(vararg commands: String): List<String> { | ||
return perform(commands) { | ||
adbTerminal.executeAdb(it) | ||
} | ||
} | ||
|
||
/** | ||
* Performs shell commands blocking current thread. | ||
* Please be aware! If any command that is in @param commands failed then AdbServerException will be thrown | ||
* | ||
* Required Permissions: INTERNET. | ||
* | ||
* @param commands commands to execute. | ||
* @throws AdbServerException if a result status of any command in @param commands is Failed | ||
* @return list of answers of commands' execution | ||
*/ | ||
override fun performShell(vararg commands: String): List<String> { | ||
return perform(commands) { | ||
adbTerminal.executeAdb("shell $it") | ||
} | ||
} | ||
|
||
/** | ||
* Disconnect from AdbServer. | ||
* The method is called by Kaspresso after each test. | ||
*/ | ||
override fun performCmd(command: String, arguments: List<String>): String { | ||
return performComplex(command, arguments, adbTerminal::executeCmd) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's better to use the single |
||
} | ||
|
||
override fun performAdb(command: String, arguments: List<String>): String { | ||
return performComplex(command, arguments, adbTerminal::executeAdb) | ||
} | ||
|
||
override fun performShell(command: String, arguments: List<String>): String { | ||
return performComplex("shell $command", arguments, adbTerminal::executeAdb) | ||
} | ||
|
||
override fun disconnectServer() { | ||
if (connected) { | ||
adbTerminal.disconnect() | ||
|
@@ -95,16 +73,27 @@ class AdbServerImpl( | |
logger.i("AdbServer. The command to execute=$command") | ||
} | ||
.map { command -> command to executor.invoke(command) } | ||
.onEach { (command, result) -> | ||
logger.i("AdbServer. The command=$command was performed with result=$result") | ||
} | ||
.onEach { (command, result) -> | ||
if (result.status == ExecutorResultStatus.FAILURE) { | ||
throw AdbServerException("AdbServer. The command=$command was performed with failed result=$result") | ||
} | ||
if (result.status == ExecutorResultStatus.TIMEOUT) { | ||
throw AdbServerException( | ||
""" | ||
.onEach { (command, result) -> logCommandResult(command, result) } | ||
.map { (_, result) -> result.description } | ||
.toList() | ||
} | ||
|
||
private fun performComplex(command: String, arguments: List<String>, executor: (String, List<String>) -> CommandResult): String { | ||
logger.i("AdbServer. The complex command to execute=$command") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
val result = executor.invoke(command, arguments) | ||
logCommandResult(command, result) | ||
|
||
return result.description | ||
} | ||
|
||
private fun logCommandResult(command: String, result: CommandResult, arguments: List<String> = emptyList()) { | ||
logger.i("AdbServer. The command=$command with arguments=$arguments was performed with result=$result") | ||
if (result.status == ExecutorResultStatus.FAILURE) { | ||
throw AdbServerException("AdbServer. The command=$command was performed with failed result=$result") | ||
} | ||
if (result.status == ExecutorResultStatus.TIMEOUT) { | ||
throw AdbServerException( | ||
""" | ||
|
||
AdbServer. The command=$command was performed with timeout exception. | ||
There are two possible reasons: | ||
|
@@ -122,10 +111,7 @@ class AdbServerImpl( | |
c. Start 'adbserver-desktop.jar' with the command in Terminal - 'java -jar /Users/yuri.gagarin/Desktop/adbserver-desktop.jar | ||
|
||
""".trimIndent() | ||
) | ||
} | ||
} | ||
.map { (_, result) -> result.description } | ||
.toList() | ||
) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General feedback.
I guess we can use
Runtime::exec(String[] cmdarray)
instead ofRuntime::exec(String command)
inCmdCommandPerformer.perform
. Have a look atjava.lang.Runtime
, where both methods call the same method andcommand
param inRuntime::exec(String command)
is parsing into the array of strings.I don't support introducing separate classes like
ComplexAdbCommand
. I think we need to add the new constructors likeCommand(command: String, arguments: List<String>)
and the correspondent methods inAdbTerminal
and etc.I don't support syntaxis like
(command: List<String>)
and(command): String
. It's confusing. Also, the most popular syntaxis in these cases is(command: String, arguments: List<String>)
. Yes, another command is an argument of the main command. Cases likesh -c "adb shell dumpsys deviceidle | grep mForceIdle"
should be described in the documentation and examples.