feat: support instrumentation capabilities #998
Conversation
alexplischke
changed the title
There was a problem hiding this comment.
This will also require updates in https://github.com/saucelabs/sauce-docs 🙏
| @@ -257,9 +257,14 @@ func (r *EspressoRunner) newStartOptions( | |||
| // Overwrite device settings | |||
| RealDeviceKind: strings.ToLower(espresso.Android), | |||
| AppSettings: job.AppSettings{ | |||
| AudioCapture: s.AppSettings.AudioCapture, | |||
| InstrumentationEnabled: s.AppSettings.ResigningEnabled, | |||
There was a problem hiding this comment.
Isn't that supposed to be InstrumentationEnabled?
| InstrumentationEnabled: s.AppSettings.ResigningEnabled, | |
| InstrumentationEnabled: s.AppSettings.InstrumentationEnabled, |
There was a problem hiding this comment.
This AppSettings to job.AppSettings struct mapping maps saucectl yaml values to composer API call json payload. In the yaml, the definition ResigningEnabled is used both for iOS and Android but in the API call, the name is different per platform. I wish I was able to rename it so that it's consistent but that's a breaking change for existing saucectl users, so we do this ugly mapping as the lesser evil.
History:
- saucectl always used
ResigningEnabledfor this value - all internal APIs always used different names for this and thus, Android instrumentation in saucectl was actually always broken if the instrumentation flag is turned off in app settings. Most people turn it on via app settings anyway so that was fine but now that we're removing app storage flag evaluations for automated tests and will use the saucectl yaml as the source of truth, this bug had to be fixed.
TLDR: The shape of that line is intentional, to avoid breaking all YAML files out there.
There was a problem hiding this comment.
We're updating the docs today 👍
There was a problem hiding this comment.
Thanks @diegoperini! It'd be good to leave an inline comment in the code explaining that little difference. I won't remember this in 2 weeks (or any new dev that joins) 😂
|
diegoperini
force-pushed
the
rdx-484-support-instrumentation-capabilities
branch
from
d7caa37 to
aef763e
Compare
| sc.Bool("audioCapture", "suite::appSettings::audioCapture", false, "Overwrite app settings for real device to capture audio.") | ||
| sc.Bool("imageInjection", "suite::appSettings::instrumentation::imageInjection", false, "Overwrite app settings for real device to inject provided images in the user app.") | ||
| sc.Bool("bypassScreenshotRestriction", "suite::appSettings::instrumentation::bypassScreenshotRestriction", false, "Overwrite app settings for real device to enable screenshot restriction.") | ||
| sc.Bool("groupFolderRedirect", "suite::appSettings::instrumentation::groupFolderRedirect", false, "Overwrite app settings for real device to redirect group folder.") |
There was a problem hiding this comment.
Hi @diegoperini ! I noticed that some iOS-specific configuration fields have been added here for Espresso run arguments. Could you clarify why these are necessary? I don't think they’re needed in this context.
There was a problem hiding this comment.
They are not needed. Addressed here: https://github.com/saucelabs/saucectl/pull/999/files
| sc.Bool("audioCapture", "suite::appSettings::audioCapture", false, "Overwrite app settings for real device to capture audio.") | ||
| sc.Bool("imageInjection", "suite::appSettings::instrumentation::imageInjection", false, "Overwrite app settings for real device to inject provided images in the user app.") | ||
| sc.Bool("bypassScreenshotRestriction", "suite::appSettings::instrumentation::bypassScreenshotRestriction", false, "Overwrite app settings for real device to enable screenshot restriction.") |
There was a problem hiding this comment.
Same applies here. bypassScreenshotRestriction is Android only. I don't think it's necessary as well.
There was a problem hiding this comment.
Addressed here: https://github.com/saucelabs/saucectl/pull/999/files
| "description": "Overwrite app settings for real device to inject provided images in the user app.", | ||
| "type": "boolean" | ||
| }, | ||
| "bypassScreenshotRestriction": { |
There was a problem hiding this comment.
Since the configuration fields are newly added and already separated into Android and iOS, I believe it would be better to split them into distinct sections for each platform in the JSON schema.
There was a problem hiding this comment.
I agree but that's a change that will make existing saucectl configurations invalid. We'll have to plan for it with proper communication.
Description
Added new instrumentation capabilities.
saucectlwill forwards these arguments in the API call it makes to further internal services.