Browserstack Turboscale Observability Integration#13437
Browserstack Turboscale Observability Integration#13437christian-bromann merged 6 commits intowebdriverio:mainfrom
Conversation
| if (process.env.BROWSERSTACK_TURBOSCALE) { | ||
| this._turboScale = process.env.BROWSERSTACK_TURBOSCALE === 'true' | ||
| } | ||
| process.env.BROWSERSTACK_TURBOSCALE_INTERNAL = this._turboScale + '' |
There was a problem hiding this comment.
Why the + '' can we properly type _turboScale?
There was a problem hiding this comment.
_turboScale is meant to be of type boolean so to have the environment variable use this, I've added an explicit cast to data type string.
| if (this._observability) { | ||
| patchConsoleLogs() | ||
|
|
||
| process.env.BROWSERSTACK_OBSERVABILITY_PRODUCT = this._isAppAutomate() ? 'app-automate' : (process.env.BROWSERSTACK_TURBOSCALE_INTERNAL === 'true' ? 'turboscale' : 'automate') |
There was a problem hiding this comment.
Can we refactor this into a utility function that takes WebdriverIO.Capabilities and returns the value for the product? Writing into environment variables to pass along data across multiple classes is a not a good approach
There was a problem hiding this comment.
That was the intention however we don't make modifications to WebdriverIO.Capabilities when running sessions on the turboscale grid so we can't use those capabilities to fetch the product name.
There was a problem hiding this comment.
You can have 3 static utility methods, e.g.
export function isAppAutomate(caps: WebdriverIO.Capabilities): boolean {
return !!caps['appium:app'] || !!caps['appium:options']?.app
}
export function isTurboScale(options: BrowserstackConfig): boolean {
return Boolean(options.turboscale)
}
export function getProduct(options: BrowserstackConfig, caps: WebdriverIO.Capabilities): boolean {
return isAppAutomate(caps)
? 'app-automate'
: (isTurboScale(options) ? 'turboscale' : 'automate')
}I strongly discouraged to just apply values to environment variables as you have no control as to if or when these values are assigned.
There was a problem hiding this comment.
I have made the code changes recommended above.
eslint-plugin-wdio
@wdio/allure-reporter
@wdio/browserstack-service
@wdio/cli
@wdio/browser-runner
@wdio/config
@wdio/concise-reporter
@wdio/cucumber-framework
@wdio/firefox-profile-service
@wdio/dot-reporter
@wdio/globals
@wdio/json-reporter
@wdio/jasmine-framework
@wdio/local-runner
@wdio/lighthouse-service
@wdio/junit-reporter
@wdio/logger
@wdio/protocols
@wdio/mocha-framework
@wdio/repl
@wdio/runner
@wdio/reporter
@wdio/shared-store-service
@wdio/sauce-service
@wdio/smoke-test-cjs-service
@wdio/smoke-test-reporter
@wdio/smoke-test-service
@wdio/spec-reporter
@wdio/static-server-service
@wdio/sumologic-reporter
@wdio/types
@wdio/utils
@wdio/testingbot-service
@wdio/webdriver-mock-service
webdriver
webdriverio
@wdio/appium-service
commit: |
|
@christian-bromann I've addressed the comments. Please re-review. |
|
@christian-bromann I've made changes as per the review comments shared above. Please re-review. |
| private _cbtQueue: Array<CBTData> = [] | ||
|
|
||
| constructor (private _browser: WebdriverIO.Browser | WebdriverIO.MultiRemoteBrowser, isAppAutomate?: boolean, private _framework?: string, _userCaps?: Capabilities.ResolvedTestrunnerCapabilities) { | ||
| constructor (private _browser: WebdriverIO.Browser | WebdriverIO.MultiRemoteBrowser, isAppAutomate?: boolean, private _framework?: string, _userCaps?: Capabilities.ResolvedTestrunnerCapabilities, _options?: BrowserstackConfig & BrowserstackOptions) { |
There was a problem hiding this comment.
Can we refactor isAppAutomate as well as the function doesn't require any scope and only takes a capability object. There is not reason to pass this into the class if you have the capabilities given here as well
There was a problem hiding this comment.
I checked the capabilities object we have inside this class & that does not have any key value pair we can use to identify if the session is an app automation session. Hence we're making use of the isAppAutomate boolean.
There was a problem hiding this comment.
Actually since the browser & the capability object are both passed here, we can simply move that method over here instead of passing the boolean inside the constructor.
There was a problem hiding this comment.
I've moved the isAppAutomate method from service.ts to insights-handles.ts & as a result, we're not passing the boolean isAppAutomate to the constructor of this class.
|
@christian-bromann are we good here? Can this be merged and deployed? |
|
@christian-bromann Could you please help us know by when can we expect this to be deployed for all v7, v8, v9? |
|
@amaanbs @pri-gadhiya can we please re-apply the feedback to the other PRs? |
|
@christian-bromann I've incorporated the changes suggested here into the PRs to be merged in branches V7 & V8. |
Proposed changes
These changes are to enable Browserstack Observability Reporting for users running sessions on Browserstack Turboscale.
Types of changes
Checklist
Backport Request
//: # (The current
mainbranch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against thev8branch.)v9and doesn't need to be back-portedFurther comments
Reviewers: @webdriverio/project-committers