Sauce: Implement CopyDeviceItem, log file parameter for RunApplication & fix issues #32
Conversation
limbonaut
changed the title
limbonaut
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
- Add optional LogFilePath parameter to Invoke-DeviceApp and RunApplication - Implement log file retrieval with fallback to system logs in SauceLabsProvider - Add method overloads to AdbProvider and XboxProvider for compatibility - Reduce code footprint by consolidating log retrieval logic - Maintain backward compatibility with existing provider implementations Co-authored-by: Claude Sonnet <[email protected]>
limbonaut
changed the title
- Replace method overloads with single method using optional LogFilePath parameter - Update base DeviceProvider class to include optional parameter - Simplify all provider implementations (SauceLabs, Adb, Xbox) - Remove conditional logic from Invoke-DeviceApp - single method call - Clean up code by eliminating method overload complexity Co-authored-by: Claude Sonnet <[email protected]>
- Add optional LogFilePath parameter to match other providers - Ensures consistent method signature across all provider implementations - Completes the refactoring to use optional parameters instead of overloads Co-authored-by: Claude Sonnet <[email protected]>
|
|
On it! We are reviewing the PR and will provide feedback shortly. |
|
|
||
| # Split the arguments string by spaces, but handle quoted strings (both single and double quotes) | ||
| $argTokens = [regex]::Matches($Arguments, '(\"[^\"]*\"|''[^'']*''|\S+)') | ForEach-Object { $_.Value.Trim('"', "'") } | ||
|
|
||
| foreach ($token in $argTokens) { | ||
| $argumentsArray += $token | ||
| } | ||
|
|
||
| $launchBody['arguments'] = $argumentsArray |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Even better, how about we change $arguments param to [object] and let users pass string or array?
There was a problem hiding this comment.
Actually, [string[]] actually accepts both a string and an array of strings
There was a problem hiding this comment.
We have multiple places where we represent arguments as a single string. I suppose I can refactor all of that in this PR.
There was a problem hiding this comment.
you shouldn't really need to change that. a single string works the same as an array of a a single string
There was a problem hiding this comment.
Ideally, we'd want to use arrays to eliminate the guesswork. But it clashes with command format string substitutions in device providers. I've converted most of our pathways to use an array of arguments, but I'm still a bit conflicted about it.
There was a problem hiding this comment.
I assume you're only changing this for the invoke-app flow - I'd just join the string by a space by default, no escaping, quoting, etc.
If someone needs custom escaping or quoting, they can always pass a single string.
let's keep it simple
There was a problem hiding this comment.
I would expect this to work:
Invoke-App -Exe "Game.exe" -Arguments @("start-level", "Big Mountain")But I get your point, and we can remove the safety. If we don't want to add such guarantees, we should clearly communicate such expectations. I'll add it to the docs then.
There was a problem hiding this comment.
I mean if you want, you can do some simple quoting but I probably wouldn't bother. It's not all that usual to have spaces in args for what we do here.
There was a problem hiding this comment.
We can leave it as is. Is there anything else we should address before it can be merged?
vaind
left a comment
vaind
left a comment
There was a problem hiding this comment.
I've found a couple of bugs and a few improvement ideas
Updated device provider APIs to consistently accept arguments as string arrays rather than pre-formatted strings, improving type safety and eliminating argument parsing ambiguities. Added ConvertArgumentsToString method to DeviceProvider base class for consistent string conversion when needed by underlying tools. Co-authored-by: Claude <[email protected]>
Also relax Intent extras validation to only validate known patterns
providers that don't support the LogFilePath parameter, and clarify in documentation that it's currently only supported on SauceLabs platforms.
limbonaut
force-pushed
the
limbonaut-ios-sauce
branch
from
7988845 to
ad71c2a
Compare
|
|
I tested it on iOS in Sauce Labs, and it should work on Android too. This PR also fixes some issues specific to iOS in Sauce Labs runner: splitting CLI arguments correctly and using correct Appium parameter for iOS.
These changes are sufficient to get E2E tests green on iOS in the Godot SDK.