Teach Cucumber::Core::Test::Case to match on parent locations. #247
Conversation
botandrose
force-pushed
the
match_parent_locations
branch
from
e4e5b7a to
39b7fa2
Compare
|
Hi @botandrose As/when I get around to releasing a v12 version of this gem (Nothing huge changing don't worry). This PR will need a rebase and slight fix / re-gen of the rubocop TODO file. I'll then plan during the v12 phase to start clicking off some of the offenses (Which number 2k+) |
|
Hi @luke-hill! Sounds good... This PR Is waiting on cucumber/gherkin#89 to be merged and released anyways. To make sure I understand, you're expecting to release v12.0 before merging this, and then merge it in during the v12.x cycle? |
|
@botandrose Ideally yes. But maybe there will be a v13 before then. this gem has been long abandoned so there's a lot of refactoring to do here. |
botandrose
force-pushed
the
match_parent_locations
branch
from
0b2c471 to
4774386
Compare
This will allow it to match on lines like Feature:, Background:, and Rule:.
botandrose
force-pushed
the
match_parent_locations
branch
from
4774386 to
c411d6e
Compare
|
@luke-hill Okay, I've rebased this on current main, resolved the merge conflicts, and the bumped the gherkin dependency, rather than my git branch in the Gemfile. Everything is green, and I think its ready for merge! What do you think? |
|
So I'm not holding you up, if I don't get any traction on moving the cck forward I will start reviewing this in a week or so. As a heads up, the next core version will be v13 so this will be in it as well. The next version of cucumber (v9.0.3), won't include this. We'll need to give it some time for the other refactors to be used first. |
lib/cucumber/core/compiler.rb
Outdated
| @@ -68,6 +71,10 @@ def source_lines_for_pickle(pickle) | |||
| pickle.ast_node_ids.map { |id| source_line(id) } | |||
| end | |||
|
|
|||
| def source_lines_for_pickle_parents(pickle) | |||
| gherkin_query.scenario_parent_locations(pickle.ast_node_ids[0]).map(&:line) | |||
There was a problem hiding this comment.
Just briefly analysing this, is there a reason we're using index 0 here. I've not dived into the code enough. If you can paste something in here to explain this it'll speed it up for me.
There was a problem hiding this comment.
@luke-hill Great question, took me a minute to figure out why. So the pickle variable represents a scenario, and usually a scenario only has one corresponding line in the document: the line that starts with Scenario:, so we would expect the accessor to actually be #ast_node_id. However, there are circumstances in which a Scenario can have multiple lines, for example:
Scenario Outline: eating
Given there are <start> cucumbers
When I eat <eat> cucumbers
Then I should have <left> cucumbers
Examples:
| start | eat | left |
| 12 | 5 | 7 |
| 20 | 5 | 15 |
In this case, the scenario has two lines, the line beginning with Scenario Outline: and then also the line containing the values, e.g. | 12 | 5 | 7 |. Therefore, the pickle has to have an Array accessor of #ast_node_ids.
However, in our use-case, finding the parent lines of a given scenario, we don't care which scenario line is used, because they both have the same parents, and there's always at least one, so we can just grab the first one with [0].
That's a lot of explanation! We should probably put this in the code somewhere? Maybe extract a well-named variable?
def source_lines_for_pickle_parents(pickle)
first_scenario_line_ast_node_id = pickle.ast_node_ids.first # Scenario Outlines have multiple lines per scenario, but they both have the same parents
gherkin_query.scenario_parent_locations(first_scenario_line_ast_node_id).map(&:line)
end
WDYT?
There was a problem hiding this comment.
@luke-hill Taking another look at this, if we change #matching_locations to a Set, rather than an Array, we could just pickle.ast_node_ids.flat_map(&:line) and call it a day. Set semantics would get rid of the duplicates. Which, now that I think about it, honestly wouldn't really be an issue, if we just left it as an array. Who cares if there's duplicate lines? The array just needs to match the given line number or not.
There was a problem hiding this comment.
Given this is an ancient piece of buggy functionality, I'm happy to go with whatever is easier.
But I'm trying to nuke huge 100's / 1000's of rubocop offenses. So if this is something you think would be better placed dealt with easily now and then refactored later I'm happy if you think we'll get to it later.
lib/cucumber/core/compiler.rb
Outdated
| @@ -38,8 +38,11 @@ def create_test_case(pickle) | |||
| uri = pickle.uri | |||
| test_steps = pickle.steps.map { |step| create_test_step(step, uri) } | |||
| lines = source_lines_for_pickle(pickle).sort.reverse | |||
| location = Test::Location.new(uri, lines) | |||
There was a problem hiding this comment.
If these are becoming procedural lets try isolate a few into something a bit more private. I'm only briefly here for a few days though so I might be wrong
There was a problem hiding this comment.
@luke-hill I think you're right. I just pushed another commit moving more of these details into private methods. If we move to a kwargs constructor for Test::Case and Test::Step we can reduce this even further by omitting the variable assignment, which is mostly for constructor arg readability.
| expect(receiver.test_case_locations).to eq test_cases.map(&:location) | ||
| end | ||
|
|
||
| it 'matches the background line to all scenarios' do |
There was a problem hiding this comment.
matches the background line (of the feature), to all scenarios
| it 'matches the location on a background step to all scenarios' do | ||
| location = Test::Location.new(file, 3) | ||
| filter = described_class.new([location]) | ||
| compile([doc], receiver, [filter]) | ||
| expect(receiver.test_case_locations).to eq test_cases.map(&:location) | ||
| end | ||
|
|
||
| it 'matches the rule with background line to all of its scenarios' do |
There was a problem hiding this comment.
matches the rule line (containing a background), to all of the rule scenarios
| ] | ||
| end | ||
|
|
||
| it 'matches the location on a rule background step to all of the rules scenarios' do |
There was a problem hiding this comment.
not sure what we mean by location? Do we just mean any GWT boomf?
There was a problem hiding this comment.
Looks like we're using "line" and "location" interchangeably. It's a line from a human perspective, but since this is testing the Test::LocationsFilter class in isolation, the parameter is a Test::Location, and the return is a Test::Location, I guess we should normalize this to "location." I'll make a pass and try to bring consistency to all the spec names, including your feedback above.
There was a problem hiding this comment.
@luke-hill Okay, pushed another commit tightening up the spec nomenclature.
|
|
||
| Rule: A rule without a background | ||
| Scenario: with a rule and no background | ||
| Given rule a |
There was a problem hiding this comment.
can we rename this portion of the test just for documentation purposes, to say something like Given a rule without a background
| Given background | ||
|
|
||
| Scenario: with a rule and background | ||
| Given rule a |
There was a problem hiding this comment.
Could we rename these two steps to be Given a rule with a background
| location = Test::Location.new(file, 29) | ||
| filter = described_class.new([location]) | ||
| compile [doc], receiver, [filter] | ||
| expect(receiver.test_case_locations).to eq [ |
There was a problem hiding this comment.
I'm slowly updating the styles of eq to use brackets (We cannot use the rubocop linting yet owing to version).
i.e. .to eq(value)
|
@luke-hill Okay, two new commits pushed. |
| test_case_named('one').location, | ||
| test_case_named('two').location | ||
| ]) | ||
| test_case_named('one').location, |
There was a problem hiding this comment.
You should be able to have these 1 indented. I think the issue is the [] chars tripping it up.
There was a problem hiding this comment.
This should work
expect(receiver.test_case_locations).to eq(
[test_case_named('one').location, test_case_named('two').location]
)
Or multiline
expect(receiver.test_case_locations).to eq(
[
test_case_named('one').location,
test_case_named('two').location
]
)
There was a problem hiding this comment.
Honestly, I think that's worse, because its less clear that there are two elements in the array, and we lose symmetry with the multiline input arrays. For example:
it 'sorts by the given locations' do
locations = [
Test::Location.new('features/test.feature', 6),
Test::Location.new('features/test.feature', 3)
]
...
expect(receiver.test_case_locations).to eq([
Test::Location.new('features/test.feature', 3),
Test::Location.new('features/test.feature', 6)
])
endI gotta say, I'm not super sold on Rubocop's utility, overall. I recognize the value of having an automated single style enforcer in an open source project with many different contributors, but here we are, having a style discussion anyways, despite Rubocop being here, trying to figure out how to game around it.
There was a problem hiding this comment.
Here's a concrete suggestion, rather than just me complaining :)
How about we disable (or make an exception for) the rule that prompted the excessive indentation in the first place?
|
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:
On behalf of the Cucumber core team, |
|
@botandrose I just want to re-iterate what the Aslak-bot said there... you've done some amazing work on these PRs, and we'd love to have you stick around and keep helping out with this code. If you want to chat any more about that, you can book me at https://calendly.com/mattwynne or find me in our community Slack. |
|
Hey Matt, thank you very much! That's such a nice message to receive. I definitely enjoyed pairing with you on this a year ago or so, so thanks for that, as well! Cucumber is one of the mainstays of my dev toolbox, and I'm happy to keep investing back into it, and I'll certainty continue to do so when I run into issues myself. I was toying with the idea of expanding my involvement with Cucumber, and making a habit of spending a few hours every weekend contributing beyond addressing my own issues. But I have to admit that a few factors have got me feeling a bit discouraged.
I don't want to imply that there has been anything but superb conduct from anyone! I'm in no way feeling that anything was unjust or incorrect or bad or wrong. But I'm simply more motivated to work on projects where I can ship more code more quickly. |
|
FYI Length of time was due to the previous full-time maintainer Aurelien no longer working full-time for Smartbear (I think previously he will have tracked most things, whereas I now do this ad-hoc using Excel). Things have changed a lot in the last few months. I'm now back here actively, so if you're wanting to contribute more I'm able to help/support reasonably promptly. |
|
I appreciate the feedback @botandrose, and yes I enjoyed pairing with you too. As Luke says we've been though a bit of a tumultuous time recently, and although that situation is hopefully drawing to a close, it has been quite disruptive and distracting for us. Sorry if you didn't get the attention you needed. One thing I would say, in reference to being able to ship stuff, is that we encourage everyone with commit rights to the |
|
Hey Luke, Matt, thanks for the responses! Really encouraging to hear. I read your blog post with some sadness, Matt, when you first posted it. So I figured those events were a big factor. And I can definitely understand how it was disruptive to managing OSS development here, to say the least. I am also pleasantly surprised to see you back in here again! Also very good to understand the Ship / Show / Ask protocol that is followed here! Consider my concerns to be completely addressed. I'll see if I can't pitch in some time every weekend or so. I'm looking forward to more collaboration together with both of you, here! |
|
If you're interested in finding out a bit more (Either async or sync), we have a weekly document that updates what we're delivering and we aim to try get together to chat for 30-45mins each week If you find us on slack we can put you in touch with our weekly zoom/g-doc that contains more info. Again no pressure if you don't want to / aren't able to, just figured I'd mention what's there. |
Description
Teach Cucumber::Core::Test::Case to match on parent locations. This will allow it to match on lines like
Feature:,Background:, andRule:. This particular strategy was identified as potentially reasonable while pairing with @mattwynne.This is the second half of a two-repo PR (including cucumber/gherkin#89) that ultimately resolves the remaining bits of cucumber/cucumber-ruby#1469
Type of change
Checklist:
bundle exec rubocopreports no offensesIf this looks favorable, and cucumber/gherkin#89 gets merged, I'll update the PR to remove the Gemfile diff pointing at that branch, and resolve the remaining checklist items above.