Fix DataTable's Location to be aware of all of its lines

[botandrose]

Hi folks! I'm back with another DataTable-related bugfix. When being built, DataTables would build their Ast::Location with only the first line. In the following example feature, this led to commands like cucumber features/test.feature:5 working, but cucumber features/test.feature:6 not running any tests, because no Test::Case reported containing that line.

Scenario: Run scenario with a data table
  Given a file named "features/test.feature" with:
    """
    Feature: 

      Scenario: DataTable
        Given this step is a table step
          | a | b |
          | c | d |
    """
  When I run `cucumber features/test.feature:6 --format pretty --quiet`
  Then it should pass with exactly:
    """
    Feature: 

      Scenario: DataTable
        Given this step is a table step
          | a | b |
          | c | d |

    1 scenario (1 passed)
    1 step (1 passed)
      
    """

How Has This Been Tested?

I'm not quite sure how best to test this. My first inclination is to add a unit test, but there appear to be no unit tests in this project for Cucumber::Core::Gherkin::AstBuilder or any of its internals. The cucumber acceptance test above could be added to the parent cucumber/cucumber-ruby project, but it seems a bit too edge-casey for a full acceptance test, IMO. Thoughts?

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:

• I've added tests for my code
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[mattwynne]

Hello @botandrose!

It looks like the rest of the specs for location matching are here: https://github.com/cucumber/cucumber-ruby-core/blob/master/spec/cucumber/core/test/filters/locations_filter_spec.rb

Maybe just add another example after this one?

[botandrose]

@mattwynne Thanks for the pointer! Indeed, that appears to be exactly the right place to add tests for this. I just rebased, squashed, and force-pushed.

As an aside, I've only recently gotten the commit-bit. What's the etiquette for merging PRs? Can I merge my own PRs in clear-cut situations like this?

[aslakhellesoy]

Hi @botandrose - I've reviewed this and I approve! Nice fix, good tests and clean code!

Go ahead and merge. I'd like to review a few more pull requests before we offer you a seat by the core team if you don't mind. Then you'll be free to merge your own pull requests (reviews will be at your discretion).

Keep up the good work!

[botandrose]

Roger that! Thanks for the review!

[aslakhellesoy]

Hi @botandrose,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

[aslakhellesoy]

@botandrose - one more thing. Please update the changelog - you can do it on master. Add an entry to the top.

Thanks
Aslak

[botandrose]

Sorry for the delay... just got back into town from the eclipse 🌑! HISTORY.md is updated.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.