[v8] Support: Setting session status and name when using BrowserStack Automate TurboScale by nagpalkaran95 · Pull Request #11487 · webdriverio/webdriverio

nagpalkaran95 · 2023-10-20T07:46:32Z

Proposed changes

Current behaviour - setting session status and name doesn't work if user is running a test on BrowserStack Automate TurboScale grid
The changes in PR adds that support

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

…mate TurboScale

erwinheitzman

Thank you for adding this 👍

erwinheitzman · 2023-10-20T10:50:05Z

packages/wdio-browserstack-service/src/service.ts

        }
+
+        if (process.env.BROWSERSTACK_TURBOSCALE) {
+            this._turboScale = true


Why are both the options and the environment variable used to set the _turboScale property?

Two behaviours, user can either pass turboScale: true/false in service options as well as just by setting up the env var.
We want env var to override the one set in config.

Just to correct, I will be updating the above statement (L62) to this._turboScale = process.env.BROWSERSTACK_TURBOSCALE === 'true'

The environment variable is a requirement by Browserstack is it not? If that is true, I think it makes sense that we set the variable for the user when the option is passed through the options object instead (I feel this is needed but I might be wrong here)

@erwinheitzman It's not mandatory. TurboScale can be set to true by either setting the environment variable or by setting turboScale key in options in config. So if any one of them is set, we use that and execute the behaviour based on that

@erwinheitzman did you get a chance to see the above response? Let me know if it clears your question or not.

packages/wdio-browserstack-service/src/types.ts

christian-bromann

LGTM 👍

christian-bromann · 2023-10-20T16:55:37Z

@erwinheitzman let me know if you have any objections to merge.

erwinheitzman · 2023-10-20T18:14:58Z

@erwinheitzman let me know if you have any objections to merge.

Not really hard objections but I believe that the environment variable that I pointed out is mandatory by Browserstack for this to work so in that case I think it makes sense to set the variable for the user if they configure the service to enable it through the service options

christian-bromann · 2023-10-25T01:32:00Z

I will go ahead and merge the PR since the questions was addressed reasonably. At this point the service goes deep into the Browserstack product offering which is great but makes it hard to for us to review the changes. @nagpalkaran95 I urged you and your team to start transitioning the service to the BS org as I don't think the WebdriverIO community should maintain it anymore given the complexity of it.

nagpalkaran95 · 2023-10-25T05:03:16Z

@christian-bromann Acknowledged. I will discuss this internally with the team once and get back on how to proceed with the transitioning. Will reach out in case of any other questions.

Meanwhile created a PR for v7 changes as well - #11527

yashdsaraf · 2023-12-19T08:14:30Z

Hey @christian-bromann, I'm part of the Browserstack SDK team. We have discussed this thoroughly within the team and we agree with your concern about the changes being made by Browserstack engineers, which require a deeper understanding of the product. But the impact of transitioning the service outside of the WebdriverIO umbrella seems like a large step that will create more ripples in terms of the code quality context needed from us and accommodating newer WebdriverIO changes (from the main branch) in the service.

An alternate approach I'd like to propose is that all the changes being made by Browserstack engineers will be reviewed by the Browserstack SDK team (which is responsible for maintaining Browserstack SDK and owns a lot of changes being done in external services like WebdriverIO), who will ensure that the coding standards required by WebdriverIO are followed and that the goal of the change in terms of Browserstack's product offering is achieved.
Post that, we will only need the WebdriverIO community to review for missing code quality pieces, if any. We're of course open to any suggestions you have to improve our review process to reduce your review burden.

Please let me know if the above approach addresses your concern; if it does, we'll start communicating internally that all changes have to be reviewed by the SDK team first.
Down the road, if you still have concerns about the review burden of the changes coming to the Browserstack WebdriverIO service, please let me know, and we can either tweak the process or explore the possibility of moving the service outside of the WebdriverIO umbrella.

(FYI @karanshah-browserstack)

christian-bromann · 2023-12-19T16:42:43Z

@yashdsaraf thanks for the response. I have no objections to keep the service as part of the core project since the BrowserStack team has shown commitment in maintaining the code. It is in the interest of the WebdriverIO core team to define certain coding rules within e.g. EsLint or introduce code formatting tools like Prettier to reduce code review time on these simple things.

Please be aware that we can't push any PRs through to meet a certain deadline, reviews by the maintenance team might take time as not everyone is online or available. In case a member of the BrowserStack team becomes a project maintainer or maybe even a TSC member, PRs to the service should still be reviewed by a non-BS member to ensure the code changes meet our standard.

Next to having the BrowserStack SDK team have an initial review on the PRs we would also love to see the team engage in triaging BrowserStack related issues actively.

Please let me know if you have any concerns. Thanks!

yashdsaraf · 2023-12-21T07:16:42Z

@yashdsaraf thanks for the response. I have no objections to keep the service as part of the core project since the BrowserStack team has shown commitment in maintaining the code. It is in the interest of the WebdriverIO core team to define certain coding rules within e.g. EsLint or introduce code formatting tools like Prettier to reduce code review time on these simple things.

Aligned with you completely on this.

Please be aware that we can't push any PRs through to meet a certain deadline, reviews by the maintenance team might take time as not everyone is online or available. In case a member of the BrowserStack team becomes a project maintainer or maybe even a TSC member, PRs to the service should still be reviewed by a non-BS member to ensure the code changes meet our standard.

Agreed with this @christian-bromann, I'll align other teams internally on this as well. We're also working on some PR creation guidelines from our end to help you get better context during reviews (e.g adding product decisions in PR description). We'd also like to add ourselves (SDK team) as CODEOWNERS in Browserstack wdio service so all reviews go through us before they come to the community, any thoughts on this?

Also please do let us know if us becoming maintainers will help, @karanshah-browserstack or I would be honoured to take the initiative and know more about this.

Next to having the BrowserStack SDK team have an initial review on the PRs we would also love to see the team engage in triaging BrowserStack related issues actively.

Agreed with this, please do share any instances in the past where you have noticed this not happening, we'll take a look and check if any of our processes / practices need tweaking accordingly.

Again thanks for your response @christian-bromann and your support over the years.

christian-bromann · 2023-12-21T16:27:21Z

We'd also like to add ourselves (SDK team) as CODEOWNERS in Browserstack wdio service so all reviews go through us before they come to the community, any thoughts on this?

Sounds good to me. Do you have a list of people that should get assigned to issues related to BrowserStack?

Also please do let us know if us becoming maintainers will help, @karanshah-browserstack or I would be honoured to take the initiative and know more about this.

Once a sufficient number of contributions are made I am happy to invite you to join the collaborator team. See our governance document for more information. Once we have you there, you can close issues yourself.

Agreed with this, please do share any instances in the past where you have noticed this not happening

There aren't any really. Once I get a list of folks that should be notified I am happy to set up an automation to auto-assign folks on BrowserStack related issues.

yashdsaraf · 2023-12-26T06:36:28Z

Sounds good to me. Do you have a list of people that should get assigned to issues related to BrowserStack?

There aren't any really. Once I get a list of folks that should be notified I am happy to set up an automation to auto-assign folks on BrowserStack related issues.

Let me create a public team of relevant folks and share it. That'll be easier to manage.

nagpalkaran95 added 2 commits October 20, 2023 13:09

Support: Setting session status and name when using BrowserStack Auto…

250fe70

…mate TurboScale

Remove redundant semicolon

65da1ef

erwinheitzman reviewed Oct 20, 2023

View reviewed changes

Different interface for turboscale response

85a2880

christian-bromann approved these changes Oct 20, 2023

View reviewed changes

christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label Oct 25, 2023

christian-bromann merged commit eb0c529 into webdriverio:main Oct 25, 2023

nagpalkaran95 deleted the v8_ats branch October 25, 2023 05:03

Uh oh!

Conversation

nagpalkaran95 commented Oct 20, 2023

Proposed changes

Types of changes

Checklist

Further comments

Reviewers: @webdriverio/project-committers

Uh oh!

erwinheitzman left a comment

Choose a reason for hiding this comment

Uh oh!

erwinheitzman Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

nagpalkaran95 Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

nagpalkaran95 Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

erwinheitzman Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagpalkaran95 Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagpalkaran95 Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

christian-bromann commented Oct 20, 2023

Uh oh!

erwinheitzman commented Oct 20, 2023

Uh oh!

christian-bromann commented Oct 25, 2023

Uh oh!

nagpalkaran95 commented Oct 25, 2023

Uh oh!

yashdsaraf commented Dec 19, 2023

Uh oh!

christian-bromann commented Dec 19, 2023

Uh oh!

yashdsaraf commented Dec 21, 2023

Uh oh!

christian-bromann commented Dec 21, 2023

Uh oh!

yashdsaraf commented Dec 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erwinheitzman Oct 20, 2023 •

edited

Loading

nagpalkaran95 Oct 23, 2023 •

edited

Loading