ISSUE-576: Allow complex ADB server commands execution #597
Conversation
Nikitae57
requested review from
matzuk,
AzamatCherchesov,
eakurnikov and
RuslanMingaliev
as code owners
| @@ -2,4 +2,4 @@ package com.kaspersky.adbserver.commandtypes | |||
|
|
|||
There was a problem hiding this comment.
General feedback.
I guess we can use Runtime::exec(String[] cmdarray) instead of Runtime::exec(String command) in CmdCommandPerformer.perform. Have a look at java.lang.Runtime, where both methods call the same method and command param in Runtime::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 like Command(command: String, arguments: List<String>) and the correspondent methods in AdbTerminal 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 like sh -c "adb shell dumpsys deviceidle | grep mForceIdle" should be described in the documentation and examples.
| } | ||
|
|
||
| private fun performComplex(command: String, arguments: List<String>, executor: (String, List<String>) -> CommandResult): String { | ||
| logger.i("AdbServer. The complex command to execute=$command") |
| * 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.
I guess it's better to use the single perform method. Don't see why we need performComplex method.
| @@ -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.
The description is unclear for people who don't have the task context.
First of all, provide examples to show the difference between the two methods.
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.
kaspresso/src/main/kotlin/com/kaspersky/kaspresso/device/server/AdbServer.kt
Show resolved
Hide resolved
| /** | ||
| * 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.
Please provide a full link to this method.
| /** | ||
| * 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.
Please provide a full link to this method.
| @@ -50,6 +54,23 @@ fun test() = | |||
| // .... | |||
| } | |||
| ``` | |||
| This method passes each command to the java runtime which tokenizes it by whitespaces. It could be not ideal. It can't be used for the commands with | |||
| the whitespaces in their arguments (e.g. `adb pull "path/with whitespace/file"`) and it doesn't allow piping (e.g. `cat $BUILD_DIR/report.txt | grep filte > filtered.txt`). | |||
There was a problem hiding this comment.
"path/with whitespace/file" is a bit confusing.
maybe "path_with whitespase to_file" is better? but it is up to you.
| "adbServer.performCmd(\"bash\", listOf(\"-c\", \"adb shell dumpsys deviceidle | grep mForceIdle\"))" + | ||
| "which the AdbServer.performCmd(String, List<String>) does. " + | ||
| "For more details, please check out https://github.com/KasperskyLab/Kaspresso/blob/master/docs/Wiki/Executing_adb_commands.en.md", | ||
| ReplaceWith("adbServer.performCmd(*commands, emptyList())") |
There was a problem hiding this comment.
Are you sure that it is correct replacement? You put an array (*commands) where adbServer.performCmd expects command: String.
There was a problem hiding this comment.
It's the best from the worst. If you leave just commands then IDE will replace adbServer.performCmd("abc") with adbServer.performCmd(arrayOf("abc"), emptyList()) which will be highlighted as an error. I think that 90% of adbServer.perform... calls are with a signle command. For those cases, when there're several commands, then it will be replaced with adbServer.performCmd("asd", "abc", emptyList()) which is ugly too, but less possible. It's a tradeoff. I don't want to leave ReplaceWith statement with a stub, because it will upset users by erasing their precious commands
| fun performCmd(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. |
There was a problem hiding this comment.
Please unify your comments. Somewhere you write class and method name, somewhere you provide a link.
| "adbServer.performCmd(\"bash\", listOf(\"-c\", \"adb shell dumpsys deviceidle | grep mForceIdle\"))" + | ||
| "which the AdbServer.performCmd(String, List<String>) does " + | ||
| "For more details, please check out https://github.com/KasperskyLab/Kaspresso/blob/master/docs/Wiki/Executing_adb_commands.en.md", | ||
| ReplaceWith("adbServer.performAdb(*commands, emptyList())") |
There was a problem hiding this comment.
Again, I am not sure about your replacement.
| @@ -2,4 +2,7 @@ package com.kaspersky.adbserver.commandtypes | |||
|
|
|||
| import com.kaspersky.adbserver.common.api.Command | |||
There was a problem hiding this comment.
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.).
Closes #576