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

Fix issue #128 #129

Closed
wants to merge 1 commit into from
Closed

Fix issue #128 #129

wants to merge 1 commit into from

Conversation

sjieg
Copy link

@sjieg sjieg commented May 16, 2017

This fix should also be committed to version 1.5.0

Summary

See issue #128 for all reproduction information.

How Has This Been Tested?

I've run multiple scenario's locally. All data still seems to be correct and now the line number for Scenario Outlines are fixed.

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.

This fix should also be committed to version 1.5.0
@mattwynne
Copy link
Member

Well done for finding the fix @sjieg!

We will need a test to accept this patch though. You could try https://github.com/cucumber/cucumber-ruby-core/blob/master/spec/cucumber/core/ast/outline_step_spec.rb though it's quite low-level. There might be some higher-level integration tests somewhere that would be a better place to test it, but I don't have time to look for you right now.

@sjieg
Copy link
Author

sjieg commented May 16, 2017

Thanks @mattwynne.

I believe the test should be added in https://github.com/cucumber/cucumber-ruby-core/blob/master/spec/cucumber/core/compiler_spec.rb . Because it's compiler that expects the correct outline_step data to be returned to it.

compiler_spec.rb:114

  expect( visitor.test_case.location ).not_to eq visitor.test_step.location

Maybe something like this, but I have little experience writing these kind of test cases. (Gherkin/Cucumber all the way here)

Anyway, thanks for the response, let me know if there is anything else I can do to help.

@brasmusson
Copy link
Contributor

This will no doubt break formatter specs in Cucumber-Ruby, they could be updated, but it may also break behaviour we want to keep.

Copy link
Contributor

@brasmusson brasmusson left a comment

Choose a reason for hiding this comment

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

This PR cannot be merged (merge conflicts aside) because

I do not think there is a simple fix for #128, I think that the expanded step needs to know both the line of the outline step and the line of the example row, and be able to provide each of these to other classes requiring one or the other. It it quite likely that it when quacking like a step should return the outline step location, but it need also be able to provide the specific location, for outline step or example row, when asked for it.

@stale
Copy link

stale bot commented Nov 27, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 27, 2017
@stale
Copy link

stale bot commented Dec 4, 2017

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@stale stale bot closed this Dec 4, 2017
@lock
Copy link

lock bot commented Dec 4, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⌛ stale Will soon be closed by stalebot unless there is activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants