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

[Bug #7739] Fix preferScenarioName logic #7740

Merged
merged 1 commit into from Jun 22, 2022

Conversation

denise-maia-ribeiro
Copy link
Contributor

Proposed changes

Updating if method from afterScenario method to put scenario's name into this._scenariosThatRan only if test status is different from 'skipped' to make option preferScenarioName works when enabled. Fix bug #7739

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 23, 2021

CLA Signed

The committers are authorized under a signed CLA.

@denise-maia-ribeiro
Copy link
Contributor Author

@christian-bromann Could you please verify these checks that failed? I fixed the tests related to wdio-browserstack-service package and all passed.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Can you find out who implemented this in the first place and what the intention behind that implementation was?

@@ -121,7 +121,7 @@ export default class BrowserstackService implements Services.ServiceInstance {
*/
afterScenario (world: Frameworks.World) {
const status = world.result?.status.toLowerCase()
if (status === 'skipped') {
if (status !== 'skipped') {
Copy link
Member

Choose a reason for hiding this comment

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

This change seems odd to me. Someone before made this code intentionally check for skipped. I am afraid with this patch that we introduce a regression for something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christian-bromann as I said in the bug #7740 , the option preferScenarioName that has the purpose of display the scenario's name as session name in Browserstack is not working with the previous code, since it works just when scenario has status skipped, that no make sense. I checked in the history of this code and the change was made when you change the code to typescript, maybe you made a mistake when changed this part of the code.

@denise-maia-ribeiro
Copy link
Contributor Author

Hi @christian-bromann . Any updates to this PR?

@christian-bromann
Copy link
Member

Any updates to this PR?

I haven't found any time to investigate the change. I still find it odd that the current implementation is completely getting turned around. I want to make sure we don't run into any regressions.

@denise-maia-ribeiro
Copy link
Contributor Author

Any updates to this PR?

I haven't found any time to investigate the change. I still find it odd that the current implementation is completely getting turned around. I want to make sure we don't run into any regressions.

ok! Just to make it clear, the code that I changed is in the same way that original code from this file (see history e20be1f#diff-e44d73826b5a9f951211dcb5b02f9462e3c489fd708d091f881b78cc86d2f724) . The operator was changed to a wrong way when the file was migrated to typescript (see history 1a7d262#diff-e44d73826b5a9f951211dcb5b02f9462e3c489fd708d091f881b78cc86d2f724). I tested it in my project locally and the preferScenarioName option is working properly with the change that I done.

@nextlevelbeard
Copy link
Member

nextlevelbeard commented Jun 22, 2022

Hey @webdriverio/project-committers
This change makes sense, as already mentioned, someone did the mistake of changing the operator in the TypeScrit re-write. Furthermore, I originally wrote this feature flag preferScenarioName and can confirm it needs this fix.

Could we listen to @denise-maia-ribeiro and merge this?
It's been here for quite a while, the feature is awfully useful and users are filing issues.

Appreciate it.

@nextlevelbeard nextlevelbeard self-requested a review June 22, 2022 14:02
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

@nextlevelbeard thanks for bumping and sorry @denise-maia-ribeiro that this took so long.

I digged into the change history and found indeed the commit that caused the regression: https://github.com/webdriverio/webdriverio/pull/6302/files#diff-e44d73826b5a9f951211dcb5b02f9462e3c489fd708d091f881b78cc86d2f724L105-L108

With a second recommendation I am happy to move forward and merge. Thanks all for your contribution to this!

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Jun 22, 2022
@christian-bromann christian-bromann merged commit e7eea12 into webdriverio:main Jun 22, 2022
@denise-maia-ribeiro
Copy link
Contributor Author

Thank you @nextlevelbeard for support it! Finally it was merged, thank you @christian-bromann! Do you have a prevision about when it will be applied in a new @wdio/browserstack-service version?

@christian-bromann
Copy link
Member

@denise-maia-ribeiro running a release now, so any second.

@Awukam
Copy link

Awukam commented Sep 7, 2022

preferScenarioName set to true is not working in webdriverio.
Set this to true to enable updating the session name to the Scenario name.
services: ['browserstack', {preferScenarioName: true}]
This issue has been resolved, please can someone look into it please thanks

@christian-bromann
Copy link
Member

Hey @Awukam , thanks for your comment.

Just saying something doesn't work really doesn't help to understand what you are going for. I recommend to provide your expected behavior and show what the current behavior is. Furthermore it is always a good idea in Open Source to be pro active and contribute by eventually propose a pull request rather than demanding to look into something that has been resolved. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants