cucumber-expressions: fix captured empty strings being undefined

[davidjgoss]

Summary

Changes to stop captures of empty string parameters being filtered off when interrogating the group, and to the specific transform function so that falsy values are handled

Details

Added a small guard to the transform function for the "string" parameter type, so falsy values are switched for an empty string.

This was enough for the tests to pass, but at the root there was still the issue of this value being undefined, due to the logic in the getter for children in the Group class.

What children looks like when the string parameter is empty:

What children looks like when the string parameter is non-empty:

Best I can tell, the previous code used value !== null as the heuristic to filter off the first item; I've changed it to start !== undefined. Although to be fair it didn't seem to be affecting anything else, so technically the guard in the transform is enough.

Motivation and Context

Fixes #746

Essentially, empty strings are a valid value for a {string} parameter, however these were coming through to the transformer as undefined and then causing an error when the function was expecting a string with a replace method. This was called out as a regression, as it used to work.

How Has This Been Tested?

Added unit tests to CucumberExpressionTest, those all other tests passing.

Confirmed it resolves the issue on the reporter's test case project.

Types of changes

• Bug fix (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).

Checklist:

• The change has been ported to Java.
• The change has been ported to Ruby.
• The change has been ported to JavaScript.
• The change has been ported to Go.
  - [ ] The change has been ported to .NET.
• I've added tests for my code.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

I haven't checked the behaviour of the other language implementations yet - will do that next but wanted to get early review on this change.

[aslakhellesoy]

Thanks @davidjgoss! As soon as this is ported to Java/Ruby/Go as well we'll merge this to master so it can go into the next release.

[davidjgoss]

Thanks - happy to pick up the Java one as part of this PR, I'm less familiar with Ruby and Go though

[aslakhellesoy]

@davidjgoss thanks! Don't worry about Ruby and Go. We'll merge this to an empty-string-fix branch first (where we can all collaborate). Someone else (@vincent-psarga?) will pick up Go and Ruby.

[aslakhellesoy]

Correction: This is already on a branch in the main repo, so we'll all just keep working on that branch. It's prefixed js-, but that's fine - we'll update all languages on this branch :-)

[davidjgoss]

Java implementation did not suffer from the issue, but added the same tests in the same place so it's covered.

[luke-hill]

Hi all, just thought I'd chime in here.

I've used this feature before in cucumber-ruby. So I've merged in the documentation PR alongside this.

The ruby implementation currently works. i.e. a step defined as

Given I do a {string} thing will match both

I do a "fun" thing
I do a "" thing

I'm not sure on the test coverage, but I know we had some zero-or-more capture lengths when I did a lot of the testing of this when fixing up the ?! issue I did ages ago.

EDIT: Also the capture is also correct, it's not nil (I guess the ruby version of undefined in js) but an empty string ""

[davidjgoss]

Took a run at adding the tests for Go; seems to also be fine already.

[aslakhellesoy]

First foray into Go? Good stuff!

[aslakhellesoy]

I see the checkbox for Ruby above is checked, but AFAIK there are no tests proving this works on Ruby. We need tests, anectdotal evidence isn't sufficient as it can allow regressions to creep in.

Did you check the box @luke-hill?

[davidjgoss]

@aslakhellesoy ah sorry I think that was me. Unchecked.

Also .NET doesn't seem to be a thing for expressions so crossed that one off.

[luke-hill]

Yeh I think (I'm not sure when or how), that ruby and go were either fixed or were working. But again, not 100% / don't know about tests.

Fully agree we should add some, but I've merged in the docs PR, as this is default behaviour in the intended usage.

[davidjgoss]

Okay Ruby tests pushed - all langs now covered

[luke-hill]

🔝 stuff

[davidjgoss]

Am I good to merge this?

[aslakhellesoy]

Hi @davidjgoss yes please go ahead! Please remember to update CHANGELOG.md and give yourself some credit!