Skip to content
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

feat: Remove ifuse pull #1019

Merged
merged 4 commits into from Jul 30, 2019
Merged

feat: Remove ifuse pull #1019

merged 4 commits into from Jul 30, 2019

Conversation

@umutuzgur
Copy link
Member

@umutuzgur umutuzgur commented Jul 29, 2019

No description provided.

@umutuzgur
Copy link
Member Author

@umutuzgur umutuzgur commented Jul 29, 2019

@KazuCocoa I tested it with the documents option to be sure

@@ -15,32 +14,11 @@ const CONTAINER_PATH_PATTERN = new RegExp(`^${CONTAINER_PATH_MARKER}([^/]+)/(.*)
const CONTAINER_TYPE_SEPARATOR = ':';
const IFUSE_CONTAINER_DOCUMENTS = 'documents';
const CONTAINER_DOCUMENTS_PATH = 'Documents';
const IO_TIMEOUT = 30000;
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 29, 2019

Choose a reason for hiding this comment

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

I'd say the timeout to 5 minutes or even more. Folders can be quite large in size

Copy link
Member Author

@umutuzgur umutuzgur Jul 30, 2019

Choose a reason for hiding this comment

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

@mykola-mokhnach 30 seconds should be fine. It is also crazy that we map the folders into a buffer in node. How much memory do we consume then?

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 30, 2019

Choose a reason for hiding this comment

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

at least the same amount that the content of zipped folder would consume. Although, I don't have any better solution for that since selenium API does not implement streamed downloads. We must keep everything in memory, convert into base64-string and then put into the response body

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 30, 2019

Choose a reason for hiding this comment

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

with maximum speed of USB2.0 connection of about 40MB/s we could download 1.2 GB within 30 seconds. Appium's limit of request body size is 1GB. Yeah, this should suffice :)

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Can we return a message for Unexpected response OBJECT_NOT_FOUND?
like Could not find 'given/path'. Please make sure the path

lib/commands/file-movement.js Outdated Show resolved Hide resolved
@umutuzgur umutuzgur requested a review from mykola-mokhnach Jul 30, 2019
await mkdirp(folderPath);
const promises = [];
await service.walkDir(relativePath, true, async (itemPath, isDir) => {
const pathOnServer = path.join(tmpFolder, itemPath);
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 30, 2019

Choose a reason for hiding this comment

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

path.posix.join

Explanation:

The path module is platform dependent. So for example, if the code is running on Windows, the path separater is always a backslash, which is not acceptable on iOS/Android, where a normal slash is used. That is why it is always safe to explicitly set path.posix, so the target path is constructed properly independently of the host OS

Copy link
Member Author

@umutuzgur umutuzgur Jul 30, 2019

Choose a reason for hiding this comment

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

@mykola-mokhnach I'm aware of that. I left it path.join since the appium server can be run also on windows. I can change it to posix if we want to

Copy link
Member Author

@umutuzgur umutuzgur Jul 30, 2019

Choose a reason for hiding this comment

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

@mykola-mokhnach Should I do it? I'm confused if I should or not do it atm

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Jul 30, 2019

Choose a reason for hiding this comment

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

This should be done if path module is used to construct/handle paths on the target phone. If we are handling paths on the local file system where the Appium server is running then everything is fine

@umutuzgur umutuzgur merged commit a5ad63d into master Jul 30, 2019
9 of 11 checks passed
@umutuzgur umutuzgur deleted the device_pull branch Jul 30, 2019
Copy link
Member

@KazuCocoa KazuCocoa left a comment

👍

try {
await closeEvent;
} catch (e) {
throw new Error(`Couldnt push the file within the given timeout ${IO_TIMEOUT}ms`);
Copy link
Member

@KazuCocoa KazuCocoa Jul 30, 2019

Choose a reason for hiding this comment

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

s/Couldnt/Couldn't/ ?

Copy link
Member

@KazuCocoa KazuCocoa Jul 30, 2019

Choose a reason for hiding this comment

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

When I tried to upload a big file, 50mb, then could observe below messages on appium server.
Should we handle them? (just a confirmation/)

Failed to receive any data within the timeout: 10000 happened

JQCK4WucYrQMzUMwCSeNQwyGw...
[debug] [XCUITest] Executing command 'pushFile'
[debug] [XCUITest] Parsed container type: documents
(node:76465) UnhandledPromiseRejectionWarning: Error: Failed to receive any data within the timeout: 10000
    at Timeout.setTimeout (/Users/kazu/GitHub/appium-xcuitest-driver/node_modules/appium-ios-device/lib/afc/index.js:255:31)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)
(node:76465) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:76465) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:76465) UnhandledPromiseRejectionWarning: Error: Failed to receive any data within the timeout: 10000
    at Timeout.setTimeout (/Users/kazu/GitHub/appium-xcuitest-driver/node_modules/appium-ios-device/lib/afc/index.js:255:31)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)
(node:76465) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
[debug] [W3C (42d14ed6)] Encountered internal error running command: Error: Couldnt push the file within the given timeout 30000ms
[debug] [W3C (42d14ed6)]     at pushFileToRealDevice (/Users/kazu/GitHub/appium-xcuitest-driver/lib/commands/file-movement.js:237:13)
[HTTP] <-- POST /wd/hub/session/42d14ed6-3f16-4281-acce-554ad11aad3a/appium/device/push_file 500 32452 ms - 587

Pushed file was fine

khanayan123 pushed a commit to khanayan123/appium-xcuitest-driver that referenced this issue May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants