deprecating array of args support in appium service by tamil777selvan · Pull Request #10972 · webdriverio/webdriverio

tamil777selvan · 2023-08-19T10:23:18Z

Proposed changes

deprecating array of args support in appium service

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

christian-bromann

I think it would be more clear if the defragCliArgs method could be incorporated into formatCliArgs. Also can we have more tests for the new function?

christian-bromann · 2023-08-20T21:34:16Z

packages/wdio-appium-service/src/utils.ts

+export function defragCliArgs(args?: AppiumServerArguments | Array<string>) {
+    if (Array.isArray(args)) {
+        return args.reduce((acc: KeyValueArgs, currentItem, index, array) => {
+            const nextItem = array[index + 1]
+
+            if (
+                currentItem?.toString().startsWith('--') ||
+                currentItem?.toString().startsWith('-')
+            ) {
+                const [key, value] = (currentItem ?? '').toString().split('=')
+
+                if (value !== undefined) {
+                    acc[key.replace(/^-+/g, '')] = value
+                } else if (
+                    nextItem &&
+                    (nextItem?.toString().startsWith('--') ||
+                        nextItem?.toString().startsWith('-'))
+                ) {
+                    acc[key.replace(/^-+/g, '')] = true
+                } else {
+                    acc[key.replace(/^-+/g, '')] = nextItem
+                }
+            }
+            return acc
+        }, {})
+    }
+
+    return args
+}


Let's return early

Suggested change

export function defragCliArgs(args?: AppiumServerArguments | Array<string>) {

if (Array.isArray(args)) {

return args.reduce((acc: KeyValueArgs, currentItem, index, array) => {

const nextItem = array[index + 1]

if (

currentItem?.toString().startsWith('--') ||

currentItem?.toString().startsWith('-')

) {

const [key, value] = (currentItem ?? '').toString().split('=')

if (value !== undefined) {

acc[key.replace(/^-+/g, '')] = value

} else if (

nextItem &&

(nextItem?.toString().startsWith('--') ||

nextItem?.toString().startsWith('-'))

) {

acc[key.replace(/^-+/g, '')] = true

} else {

acc[key.replace(/^-+/g, '')] = nextItem

}

}

return acc

}, {})

}

return args

}

export function defragCliArgs(args?: AppiumServerArguments | Array<string>) {

if (!Array.isArray(args)) {

return args

}

return args.reduce((acc: KeyValueArgs, currentItem, index, array) => {

const nextItem = array[index + 1]

if (

!currentItem?.toString().startsWith('--') &&

!currentItem?.toString().startsWith('-')

) {

return acc

}

const [key, value] = (currentItem ?? '').toString().split('=')

if (value !== undefined) {

acc[key.replace(/^-+/g, '')] = value

} else if (

nextItem &&

(nextItem?.toString().startsWith('--') ||

nextItem?.toString().startsWith('-'))

) {

acc[key.replace(/^-+/g, '')] = true

} else {

acc[key.replace(/^-+/g, '')] = nextItem

}

return acc

}, {})

}

tamil777selvan · 2023-08-21T03:51:05Z

I think it would be more clear if the defragCliArgs method could be incorporated into formatCliArgs. Also can we have more tests for the new function?

At first, I considered changing the formatCliArgs function instead of creating a new one. However, because _args is used in _setCapabilities, which also needs the port value, I ended up needing this new method.

Also as suggested, I have updated the new method and added more tests to check it's functionality. Please share your thoughts on this.

christian-bromann · 2023-08-21T07:11:38Z

@tamil777selvan I am a bit confused. It seems to be that with defragCliArgs we now support to pass in Appium args as object and array. If this is the case I suggest to only go with one option since it can get confusing otherwise. E.g. update the docs and make sure check the type of the _args object in the service.

tamil777selvan · 2023-08-21T09:20:15Z

@tamil777selvan I am a bit confused. It seems to be that with defragCliArgs we now support to pass in Appium args as object and array. If this is the case I suggest to only go with one option since it can get confusing otherwise. E.g. update the docs and make sure check the type of the _args object in the service.

Hi @christian-bromann, The documentation explains its capability to accept objects/arrays as inputs for 'args'. Could we consider discontinuing the array support in this scenario? Please share your opinion.

christian-bromann · 2023-08-21T15:18:28Z

Could we consider discontinuing the array support in this scenario? Please share your opinion.

Yeah, I would suggest that. Given array support never worked, we can safely discontinue it. It is less confusing to only support 1 type here. Would you mind making that change?

tamil777selvan · 2023-08-22T01:40:24Z

Could we consider discontinuing the array support in this scenario? Please share your opinion.

Yeah, I would suggest that. Given array support never worked, we can safely discontinue it. It is less confusing to only support 1 type here. Would you mind making that change?

Yeah sure, on it.

tamil777selvan · 2023-08-22T03:50:48Z

Hi @christian-bromann, I've eliminated the array support for arguments and enforced the type to be an object. Please share your opinion.

christian-bromann · 2023-08-22T04:32:19Z

packages/wdio-appium-service/README.md


 See [the documentation](https://appium.github.io/appium.io/docs/en/writing-running-appium/server-args/index.html) for possible arguments.
-The arguments should be supplied in lower camel case, so `--pre-launch true` becomes `preLaunch: true` or passed as an array.
+The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing true`, or they can be supplied as outlined in the Appium documentation.


Boolean flags should not have a value.

Suggested change

The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing true`, or they can be supplied as outlined in the Appium documentation.

The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing`, or they can be supplied as outlined in the Appium documentation.

Can you link Appium documentation to an Appium resource that could be of help here?

Furthermore, the documentation link is connected in line 88; however, it points to the outdated Appium version. Would it be advisable to directly link to the documentation here on GitHub?

yes, that makes sense

christian-bromann · 2023-08-22T04:34:04Z

packages/wdio-appium-service/README.md

+                '-p': 1234,
+                '--relaxed-security': true


I don't think we should document such usage and encourage users to write out the full property name.

Understood, but would this apply equally to aliases as well?

I am not sure if we should support these, I think if someone e.g. sets { p: 1234 } we can create ['-p', 1234] out of it. What do you think?

I've observed that this approach isn't applicable to all aliases.

For example: if I have { p: 1234 }, following the formatting, it would result in ['--p', 1234], which Appium would accept.

However, consider a scenario where I provide an address in the alias, such as { a: 'foo' }. The formatting would lead to ['--a', 'foo'], triggering an ambiguous error in Appium.

To address this situation, it becomes necessary to either permit the use of full property names in the args or to consider discontinuing the support by updating the documentation. Please share your opinion.

When I look at appium server --help I get:

appium server [-h] [--address ADDRESS] [--allow-cors] [--allow-insecure ALLOW_INSECURE] [--base-path BASE_PATH] [--callback-address CALLBACK_ADDRESS] [--callback-port CALLBACK_PORT] [--debug-log-spacing] [--default-capabilities DEFAULT_CAPABILITIES] [--deny-insecure DENY_INSECURE] [--keep-alive-timeout KEEP_ALIVE_TIMEOUT] [--local-timezone] [--log LOG] [--log-filters LOG_FILTERS] [--log-level LOG_LEVEL] [--log-no-colors] [--log-timestamp] [--long-stacktrace] [--no-perms-check] [--nodeconfig NODECONFIG] [--port PORT] [--relaxed-security] [--session-override] [--strict-caps] [--tmp TMP] [--trace-dir TRACE_DIR] [--use-drivers USE_DRIVERS] [--use-plugins USE_PLUGINS] [--webhook WEBHOOK] [--shell] [--show-build-info] [--show-config] [--config CONFIGFILE]

it doesn't seem to advocate for the usage of aliases a lot. I would just suggest to not support those and make this clear in the docs.

From the user experience point of view I feel like this makes the configuration more clear anyway. What do you think?

Certainly, that sounds like a solid plan. I'll integrate those adjustments and make the necessary updates to the documentation.

Hi @christian-bromann, I have implemented all the required modifications. Please review them at your convenience.

packages/wdio-appium-service/tests/launcher.test.ts

christian-bromann

Since we are already at this, could we extend the AppiumServerArguments and actually fill it up with given server properties rather than just having: [capability: string]: any. The rest looks good to me!

packages/wdio-appium-service/src/launcher.ts

tamil777selvan · 2023-08-23T09:14:24Z

Since we are already at this, could we extend the AppiumServerArguments and actually fill it up with given server properties rather than just having: [capability: string]: any. The rest looks good to me!

I have included all the parameters and corresponding types utilised by the Appium server CLI. Please review them at your convenience.

christian-bromann

This is awesome, thank you so much 👍

defrag appium service args

0754d00

christian-bromann reviewed Aug 20, 2023

View reviewed changes

adding tests & fix issues on alias support

66d77a2

adding eofl

0f51ffd

deprecating array of args support

aa51748

tamil777selvan changed the title ~~defrag appium service args~~ deprecating array of args support in appium service Aug 22, 2023

christian-bromann reviewed Aug 22, 2023

View reviewed changes

tamil777selvan added 2 commits August 23, 2023 07:06

incorporating review comments.

f0f085d

removing unImplemented mock value

93dfb93

christian-bromann reviewed Aug 23, 2023

View reviewed changes

packages/wdio-appium-service/src/launcher.ts Outdated Show resolved Hide resolved

adding data types to AppiumServerArguments

dc7b004

christian-bromann approved these changes Aug 23, 2023

View reviewed changes

christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label Aug 23, 2023

christian-bromann merged commit 9653b03 into webdriverio:main Aug 23, 2023

christian-bromann mentioned this pull request Sep 15, 2023

[🐛 Bug]: Passing args as an array to the Appium service creates erroneous arguments #10942

Closed

3 tasks

	The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing true`, or they can be supplied as outlined in the Appium documentation.
	The arguments are supplied in lower camel case. For instance, `debugLogSpacing: true` transforms into `--debug-log-spacing`, or they can be supplied as outlined in the Appium documentation.

Uh oh!

Conversation

tamil777selvan commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Checklist

Further comments

Reviewers: @webdriverio/project-committers

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamil777selvan commented Aug 21, 2023

Uh oh!

christian-bromann commented Aug 21, 2023

Uh oh!

tamil777selvan commented Aug 21, 2023

Uh oh!

christian-bromann commented Aug 21, 2023

Uh oh!

tamil777selvan commented Aug 22, 2023

Uh oh!

tamil777selvan commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamil777selvan commented Aug 23, 2023

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tamil777selvan commented Aug 19, 2023 •

edited

Loading