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

Support for docstrings in scenario outlines #80

Conversation

christian-eriksson
Copy link
Contributor

Figured I might as well just create the pull request, I can always squash the commits later.

I've added what I hope is a solution to using Docstrings with Scenario Outlines as described in #7 and #79. The implementation is concentrated to src/parsed-feature-loading.ts I also added a couple of examples for TypeScript and ECMAScript to illustrate the new functionality (it was previously not possible to use Docstrings and Scenario Outlines together).

Have a look and ping me if there are any concerns.

scenarioStep.stepArgument instanceof String
) {
Object.keys(exampleTableRow).forEach((nextTableColumn) => {
stepArgument = (stepArgument as string).replace(
Copy link
Owner

Choose a reason for hiding this comment

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

It's possible that a doc string might have a field specified more than once, but this code will only replace one instance. It would be better to use a regex with /g to ensure each column gets replaced globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice catch that would indeed be quite possible. I'll update the tests to reflect this as well.

I believe this is a problem also with the current implementation of scenario outlines and placeholders in steps and arguments (titles use regex an works fine). Do you want me to change those into regex checks as well? (I'll just have to come up with some good test scenario where you'd write something like Given I <placeholder> <placeholder> 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix now uses regex.

@@ -0,0 +1,21 @@
Feature: Certificate manufaturing
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like a typo: manufacturing is missing the "c".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@bencompton
Copy link
Owner

@SirSkorpan - thanks for contributing this fix! I commented on a couple of small tweaks, but it looks good otherwise. Once those issues are resolved, I can merge and publish.

I will be publishing this in a beta package for now instead of in the mainline because I've been a bad maintainer the last several months, and master still has some beta functionality in it from a while back (#27).

@christian-eriksson
Copy link
Contributor Author

@bencompton I added a few tests for using repeating 's in steps, docstrings and gherkin tables. Hopefully you feel that those belong in this PR as well. As I was adding regex in the replace method for my new code I figured I might as well use it in the old code as well to add support not just to docstrings.

Also added some information to the docs about placeholders in scenario outlines as well as docstrings. Have a look an get back to me if you feel something is missing or doesn't belong in this PR.

Also minor fixed in formatting of README.md
@bencompton bencompton merged commit fdbe4cd into bencompton:master Feb 29, 2020
@bencompton
Copy link
Owner

@SirSkorpan - Thanks for fixing the replacement in both places, and also adding additional examples! Your changes are now merged and published to v2.0.13 (tagged as "next").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants