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

CucumberJS v2.X regex syntax {.*} not being detected #99

Closed
prietoguy opened this issue Jun 9, 2017 · 12 comments
Closed

CucumberJS v2.X regex syntax {.*} not being detected #99

prietoguy opened this issue Jun 9, 2017 · 12 comments
Labels
Milestone

Comments

@prietoguy
Copy link

Hi @alexkrechik,

I am a great fan of your extension. GREAT WORK! And thanks to the latest formatting functionality added in v2.3.0, I no longer have to spend time arranging those data tables. 👍

I am running into the following issue and was wondering if you could help me (solution provided below).

Given step definition:

And I should delete test data for: UI

When using CucumberJS 2.X regex syntax:

Then('I should delete test data for: {.*}', function (testType) {...})

Then CucumberJS recognizes step definition, but Extension does not:

[cucumberautocomplete] Was unable to find step for "And I should delete test data for: UI"

When I use CucumberJS 1.X regex syntax:

Then('I should delete test data for: (.*)', function (testType) {...})

Then Extension recognizes step definition, but CucumberJS does not:

  //Undefined. Implement with the following snippet:

  Given('I should delete test data for: UI', function () {
    // Write code here that turns the phrase above into concrete actions
    return 'pending';
  });

Unless I am using CucumberJS 2.X regex syntax improperly, would you consider adding the following line of code to step_handlers?

step = step.replace(/{\.\*}/g, '.*');

Thanks again for the awesome extension!

@alexkrechik
Copy link
Owner

Hi. Thank you for your feedback!!

Regarding your issue, I have one question. Should any regexp be using with "{}" symbols (ex. "{.+}" or "{value1|value2}") due to CucumberJS 2.X regex syntax?

@prietoguy
Copy link
Author

Sorry @alexkrechik, I was mistaken. AFAIK, this has more to do with Cucumber Expressions. In the example above, if you change {.*} to {zzz} CucumberJS 2.X still recognizes that step definition.

So I guess the change would be to handle any unmatched custom parameter types to a word boundary regex. Does that make sense?

@prietoguy
Copy link
Author

Eventually the extension could scan directory for custom parameter types and use its regex pattern. For now any other terms found inside {} (aside from native int and float) would match the code below:

step = step.replace(/{(.*)}/g, '.*');

I tried this locally and it works with one or more cucumber expressions. I'll submit a pull request shortly.

@prietoguy
Copy link
Author

Pull request #103 created.

@alexkrechik
Copy link
Owner

alexkrechik commented Jun 23, 2017

HI!
Thank you for your activity.
Looks like, we should change the most {(.*)} - like parts to the .*, but not all like in your merge request. I see at minimum two exceptions here:

  1. Backslashed '{' symbols should not be affected.
  2. {} symbols in regexps (like /.*{2, 3}/ or /.*{, 3}/) should not be affected

Also, there are some tests, that should be filled and passed for the such changes (I'm trying to do this for all the cases)

So, could you please do the next things (otherwise I can do this when will have some free time, likely next week):

  1. Avoid situations, when there are some backslash symbol before the '{' or when there are some number or comma after '{' symbol
  2. Add tests for all the additions, like adding next checks here:
['I use {some cucumber regex}', 'I use .*'],
['I use {.*}', 'I use .*'],
['I use \{ some backslashed thing \}', 'I use \{ some backslashed thing \}'],
['I use (.*){2,5}', 'I use (.*){2,5}'],
['I use (.*){,5}', 'I use (.*){,5}'],

@prietoguy
Copy link
Author

Hi @alexkrechik,

I finally got around to improving the cucumber expression logic and writing those test cases as you suggested. Thanks for pointing me on the right direction!

Only problem I keep running into is with the backslashed case. There must be a function that removes the backslashes. Can you take a look at it?

128 passing (88ms)
  1 failing

  1) getRegTextForStep should correctly handle cucumber expressions:

      AssertionError: expected 'I use .*' to equal 'I use { some backslashed thing }'
      + expected - actual

      -I use .*
      +I use { some backslashed thing }

@alexkrechik
Copy link
Owner

Hi! Finally I've up the tests (they were failed due to new TypeScript issue), so will be able to look into this.

@alexkrechik
Copy link
Owner

Fixed backslashes case and merged it.

@prietoguy
Copy link
Author

prietoguy commented Jul 11, 2017

Thanks @alexkrechik. Looks great! Any idea when the next update will be for the plugin?

Also, should I go ahead and close the issue?

@alexkrechik
Copy link
Owner

Hi! I should close at least one more issue - #102. After this I'll test the extension, and, if no additional regressions found, will release the path version.

@alexkrechik alexkrechik reopened this Jul 15, 2017
@alexkrechik
Copy link
Owner

Also, lets close this issue after extension release

@alexkrechik alexkrechik added this to the 2.4.0 milestone Jul 15, 2017
@alexkrechik
Copy link
Owner

Should be fixed in 2.3.1

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

No branches or pull requests

2 participants