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

Restore ability to specify a line number within a scenario. #1497

Closed

Conversation

botandrose
Copy link
Contributor

Fixes regression described in #1469

Regression
I used to be able to run cucumber feature_file_name.feature:XX with XX being a line anywhere in the scenario (usually the line that failed last time, to easily re-run a failure) but now it only works if XX is the number of the "Scenario" heading line. Additionally, running :1 used to run all scenarios, now it runs none.

Solution
Due to the internal changes that seem to make a "proper" fix invasive (see #1469 (comment)), here is hacky solution in the CLI layer. When an file argument is found with a line number specified, we open the file, inspect the specified line, iterate upwards until we see Scenario:, and then mutate the supplied line number into that line number. If we happen to find Scenario Outline: first, just leave the number as-is. If neither is found, i.e. we're above the first scenario in e.g. Background: or Feature:, then all scenarios should be run, so remove the line number completely.

Notes
This is hardly the most elegant solution (or code), but I want to solicit discussion and feedback from the cucumber dev community before iterating much further on it. What do y'all think? Is there a better way to go about fixing this regression that doesn't involve rearchitecting cucumber's internals all over again?

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Before reviewing the code part of the test. Could we have a scenario with 3 steps, and you use the 2nd step (Just so we cover our bases with future regressions).

You can add this as an additional one or modify the existing one. It just will be a bit more robust.

@botandrose botandrose force-pushed the restore_looser_line_numbers branch 3 times, most recently from 959ae5a to 347fa30 Compare January 30, 2021 01:08
@botandrose botandrose force-pushed the restore_looser_line_numbers branch from 347fa30 to d5f100f Compare January 30, 2021 01:21
@botandrose
Copy link
Contributor Author

Okay, test modified as requested! Rebased on master to resolve windows failure, too.

@luke-hill
Copy link
Contributor

Cheers. I'll have a look at the code when I next have some free time :) Unless someone else gets to it first. The tests though enable us to check this new logic still works.

@aslakhellesoy aslakhellesoy added the ⚡ enhancement Request for new functionality label Feb 2, 2021
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.

It is unclear what the intended behavior is several corner cases

  • when the line number is the line of the tag-line preceding the scenario line (the line of the scenario keyword)
  • when the line number is the line of an (outline) step in a scenario with examples (previously known as a "Scenario Outline"
  • etc
    In the end I think this kind of change needs an rspec spec to specify the behavior comprehensively.

line_numbers.map! do |line_number|
line_number.to_i.downto(1).find do |i|
line = lines[i - 1]
raise ScenarioOutlineFound if line =~ /^\s+Scenario Outline:/
Copy link
Contributor

Choose a reason for hiding this comment

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

In Gherkin today also Scenarios can have an Examples Table ("Scenario Outline" only a synonym for "Scenario"), so it is not possible to decide if it is a simple Scenario (without Examples Table) of a Scenario with examples (previously known as "Scenario Outline" only by examine the keyword for the scenario line.

line_number.to_i.downto(1).find do |i|
line = lines[i - 1]
raise ScenarioOutlineFound if line =~ /^\s+Scenario Outline:/
line =~ /^\s+Scenario:/
Copy link
Contributor

Choose a reason for hiding this comment

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

A) This does not handle feature files in other language than English.
B) This does not handle all alternatives possible in a feature file in English (in Gherkin today "Example" is an alternative to "Scenario")

@mattwynne
Copy link
Member

Thanks for your contribution @botandrose.

I've got to say, I'd much rather we fix the shortcomings in the gherkin internals that have made this awkward, rather than plastering on a hack around the outside of it. Do you have any energy to help me pursue the other avenue to get this problem solved properly?

@mattwynne mattwynne closed this May 1, 2021
@mattwynne mattwynne deleted the branch cucumber:master May 1, 2021 05:28
@botandrose botandrose deleted the restore_looser_line_numbers branch January 20, 2023 01:22
@botandrose botandrose restored the restore_looser_line_numbers branch January 20, 2023 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ enhancement Request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants