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

Fix for rule keyowrd not working 'Cannot use Rule keyword in Cucumber… #7153

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

praveendvd
Copy link
Contributor

… tests #7140'

Proposed changes

Allows to use Rule keyword in cucumber feature file

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

@christian-bromann

@praveendvd
Copy link
Contributor Author

@christian-bromann i am flattening the rule array , do we need to show the rule name in report ? should we preserve the rule array

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.

@praveendvd I would say the Rule keyword should wrap containing scenarios, so given a Gherkin file like:

@tag
Feature: Feature

  Background: Background
    Given I do stuff

  Rule: Only something

    Scenario: Doing something
      Given I do even more stuff

Should create the following spec output:

------------------------------------------------------------------
[chrome 91.0.4472.114 mac os x #0-0] Running: chrome (v91.0.4472.114) on mac os x
[chrome 91.0.4472.114 mac os x #0-0] Session ID: a540510b6d7d4173283777165cdb353c
[chrome 91.0.4472.114 mac os x #0-0]
[chrome 91.0.4472.114 mac os x #0-0] » /mocha/mocha.test.js
[chrome 91.0.4472.114 mac os x #0-0] Feature
[chrome 91.0.4472.114 mac os x #0-0]     Only Something
[chrome 91.0.4472.114 mac os x #0-0]         Doing something
[chrome 91.0.4472.114 mac os x #0-0]            ✓ Given I do even more stuff
[chrome 91.0.4472.114 mac os x #0-0]
[chrome 91.0.4472.114 mac os x #0-0] 1 passing (1.7s)

Does this make sense?

Comment on lines +202 to +203
const rules = feature.children.filter((child)=> Object.keys(child)[0]=== 'rule')
feature.children = feature.children.filter((child)=> Object.keys(child)[0]!== 'rule')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const rules = feature.children.filter((child)=> Object.keys(child)[0]=== 'rule')
feature.children = feature.children.filter((child)=> Object.keys(child)[0]!== 'rule')
const rules = feature.children.filter((child) => Object.keys(child)[0] === 'rule')
feature.children = feature.children.filter((child) => Object.keys(child)[0] !== 'rule')

is child always an object with one property? Why do we always want to access the first key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First key is either rule or scenario always

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Thanks


const rules = feature.children.filter((child)=> Object.keys(child)[0]=== 'rule')
feature.children = feature.children.filter((child)=> Object.keys(child)[0]!== 'rule')
const rulesChildrens:any = rules.map((child)=>child.rule?.children).flat()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const rulesChildrens:any = rules.map((child)=>child.rule?.children).flat()
const rulesChildrens = rules.map((child) => child.rule?.children).flat()

Copy link
Member

Choose a reason for hiding this comment

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

maybe it makes sense to change the line to rules.filter((child) => child.rule).map((child) => child.rule?.children).flat()?
Otherwise, it's possible to have an array of undefined values in case there is no child.rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgrybyk rules will have rule , the structure is like { rules : rule :children}, if there is no rules , rules will be an empty array the child won't be iterated.

Copy link
Member

@mgrybyk mgrybyk Jul 19, 2021

Choose a reason for hiding this comment

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

Let's don't use .? in such case child.rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgrybyk true , i will check the type for it . IT was giving type script error thank you :)

@christian-bromann christian-bromann added the needs-review These Pull Requests require review from project members label Jul 19, 2021
@praveendvd praveendvd closed this Jul 19, 2021
@praveendvd praveendvd reopened this Jul 19, 2021
@praveendvd praveendvd closed this Jul 19, 2021
@christian-bromann
Copy link
Member

@praveendvd why closing the PR?

@praveendvd
Copy link
Contributor Author

praveendvd commented Jul 20, 2021

@praveendvd why closing the PR?

Wanted to investigate how to propagate the rule name to the report do you want to keep the pr open till then ? @christian-bromann

@christian-bromann
Copy link
Member

Yeah, no reason to close it.

Added missing assignment
@praveendvd praveendvd reopened this Jul 20, 2021
@praveendvd praveendvd closed this Jul 20, 2021
@praveendvd praveendvd reopened this Jul 20, 2021
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.

👍 LGTM, thanks!

@christian-bromann christian-bromann added PR: Polish 💅 PRs that contain improvements on existing features and removed needs-review These Pull Requests require review from project members labels Jul 22, 2021
@christian-bromann christian-bromann merged commit ba9832e into webdriverio:main Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Polish 💅 PRs that contain improvements on existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants