Skip to content

Adopt latest Cucumber API interface#11010

Merged
christian-bromann merged 17 commits intowebdriverio:mainfrom
tamil777selvan:cucumber
Sep 1, 2023
Merged

Adopt latest Cucumber API interface#11010
christian-bromann merged 17 commits intowebdriverio:mainfrom
tamil777selvan:cucumber

Conversation

@tamil777selvan
Copy link
Member

@tamil777selvan tamil777selvan commented Aug 25, 2023

Proposed changes

These modifications enable the utilisation of the latest Cucumber API interface for execution.

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

@tamil777selvan tamil777selvan marked this pull request as draft August 25, 2023 11:57
@nextlevelbeard
Copy link
Member

nextlevelbeard commented Aug 26, 2023

Great to see work on this 👍 personal favorite feature I wanted
PS: fixes #8149

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.

Fantastic work 👏 do you see anything that could potentially make user tests break after the update?

@tamil777selvan
Copy link
Member Author

Fantastic work 👏 do you see anything that could potentially make user tests break after the update?

With these changes, the main area that will be affected is the capability to skip tests based on specific conditions. I believe this feature is widely used by many users. I'm currently working on finding a solution to address this impact and come up with an alternative approach.

@tamil777selvan tamil777selvan marked this pull request as ready for review August 28, 2023 11:10
@tamil777selvan
Copy link
Member Author

Hi @christian-bromann, I've managed to include all the current features in this by adopting the latest version of Cucumber. Kindly take a moment to review and share your thoughts on these updates.

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.

Some comments. Overall very happy to see this change 🤩

'afterTestCaseHookDefinitionConfigs',
'afterTestRunHookDefinitionConfigs',
] as const
const cucumberFormatter = path.resolve(url.fileURLToPath(import.meta.url), '../cucumberFormatter.js')
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use a file url string for windows and linux based OS?

Suggested change
const cucumberFormatter = path.resolve(url.fileURLToPath(import.meta.url), '../cucumberFormatter.js')
const cucumberFormatter = url.pathToFileURL(path.resolve(url.fileURLToPath(import.meta.url), '../cucumberFormatter.js')).href

Copy link
Member Author

Choose a reason for hiding this comment

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

At first, I took the same approach. When we use url.pathToFileURL, the file URL comes with the file:/// prefix with three slashes (///), which works well for Linux-based systems. However, Cucumber internally modifies the provided file URL and raises an error if there are three slashes (///) present on windows.

Upon further investigation, I observed that Cucumber employs the split operation, which creates issues on Windows. As a result, I've explicitly added a double slash prefix to the file path for Windows.

Copy link

@davidjgoss davidjgoss Sep 6, 2023

Choose a reason for hiding this comment

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

Sorry this is such a pain point with Cucumber!

I just released 9.5.1 of cucumber-js which includes support for expressing each format as a 1- or 2-item array instead of a string, so it's explicit what is the formatter name/path and (optionally) what is the target and there is no buggy split operation. So with the configuration below it could be something like:

format: [
  [cucumberFormatter]
]

And that value should be fine as a regular file URL regardless of platform - if you're able to give this a try, let me know how it goes.

Also more generally, feel free to start a thread in cucumber/cucumber-js#2041 with any feedback you have on the new API. I'm conscious that the docs are pretty thin still, so I'm happy to fill any gaps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @davidjgoss for handling this. I'll make the necessary changes and will get in touch if I encounter any issues.

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.

This is an excellent change set, fantastic job 👍

@christian-bromann christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Sep 1, 2023
@christian-bromann
Copy link
Member

I will look at the Windows test in main, these failures aren't related to this PR.

@christian-bromann christian-bromann merged commit e975dea into webdriverio:main Sep 1, 2023
christian-bromann added a commit that referenced this pull request Sep 1, 2023
@christian-bromann
Copy link
Member

@tamil777selvan just letting you know that you have picked up two issues with an 💸 Expensible tag which means you need 8 more of those to be able to claim $1000 from our fund.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: New Feature 🚀 PRs that contain new features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants