Skip to content

fix: tune isMobile for Appium situation#10491

Merged
christian-bromann merged 4 commits intowebdriverio:mainfrom
KazuCocoa:tune-ismobile
Jun 3, 2023
Merged

fix: tune isMobile for Appium situation#10491
christian-bromann merged 4 commits intowebdriverio:mainfrom
KazuCocoa:tune-ismobile

Conversation

@KazuCocoa
Copy link
Contributor

@KazuCocoa KazuCocoa commented Jun 3, 2023

Proposed changes

Current isMobile detection returns true if the session was appium by checking the caps has appium:. In this case, for example. the displayed check method call calls /displayed endpoint, although Appium would like to expect to get JS one for chrome/msedge (desktop browser) for https://github.com/appium/appium-chromium-driver . It could cause unexpected W3C/MJSONWP endpoint call. So this PR's idea is to make the isMobile true only when the platformName is ios, tvos and android. For Appium's case, probably the platformName is more accurate in detecting whether the target device is mobile, or not.

Btw, when I tried to run npm run test:unit to check the test code, I got an error below. Thus I haven't completed the test case yet. npx vitest ./packages/wdio-utils/tests also returned the same result.

Let me kick this repository's CI how the test case will be... (since it did not work on my local...)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Error during global setup ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: Renaming "got" dependency failed!

WebdriverIO needs to hide the "got" dependency (at "packages/webdriver/node_modules/got")
during the test to force Vitest to use our mocked version. WebdriverIO does this by renaming:

"packages/webdriver/node_modules/got"
to
"packages/webdriver/node_modules/tmp_got"

during test setup and back again during test tear-down.

'Setup has failed.  Maybe because you are already running unit tests in a different
'terminal.
Error: ENOENT: no such file or directory, rename '/Users/kazu/GitHub/webdriverio/packages/webdriver/node_modules/got' -> '/Users/kazu/GitHub/webdriverio/packages/webdriver/node_modules/tmp_got'

To correct this error please run:
        mv packages/webdriver/node_modules/tmp_got packages/webdriver/node_modules/got

 ❯ throwBetterErrorMessageSetup scripts/test/globalSetup.ts:28:9
     26|     throw new Error(
     27|         util.format(ERROR_MESSAGE, 'Setup') +
     28|         err.stack +
       |         ^
     29|         '\n\nTo correct this error please run:\n\tmv packages/webdriver/node_modules/tmp_got packages/webdriver/node_modules/got\n'
     30|     )
 ❯ Object.setup scripts/test/globalSetup.ts:38:3
 ❯ hookParallel node_modules/vite/dist/node/chunks/dep-e8f070e8.js:42512:9
 ❯ Object.buildStart node_modules/vite/dist/node/chunks/dep-e8f070e8.js:42824:13

In my brief testing on my local, iOS session was handled as isMobile as true but false for chrome desktop. This is expected behavior.

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
christian-bromann approved these changes Jun 3, 2023
View reviewed changes
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes totally sense, thank you 👍

@christian-bromann christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label Jun 3, 2023
@christian-bromann christian-bromann merged commit caa9877 into webdriverio:main Jun 3, 2023
@KazuCocoa KazuCocoa deleted the tune-ismobile branch June 3, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christian-bromann christian-bromann christian-bromann approved these changes

Assignees

No one assigned

Labels

PR: Polish 💅 PRs that contain improvements on existing features

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KazuCocoa @christian-bromann