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

Pretty Formatter Fails When Printing Expanded Scenario Outline #620

Closed
subakva opened this Issue Jan 15, 2014 · 4 comments

Comments

Projects
None yet
3 participants
@subakva

subakva commented Jan 15, 2014

It appears that StepCollection#max_line_length does not account for expanded scenario outline steps. This causes HasSteps#source_indent to return a negative value, which ultimately leads to an ArgumentError while trying to pretty print the step name.

Using:

  • ruby 2.0.0
  • cucumber 1.3.10

Given a feature like this:

Feature: 
  Scenario Outline: Viewing a Long String
    Then I see the string "<long string>"

  Examples:
    | long string |
    | Subsequently, I found that at the third episode of "Wormling" was not at all what anyone had expected. Seventeen moles were in attendance. |

And cucumber is run like this:
cucumber --format pretty --expand

It raises an error like this:

negative argument (ArgumentError)
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/activesupport-4.0.2/lib/active_support/core_ext/string/indent.rb:8:in `*'
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/activesupport-4.0.2/lib/active_support/core_ext/string/indent.rb:8:in `indent!'
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/activesupport-4.0.2/lib/active_support/core_ext/string/indent.rb:41:in `block in indent'
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/activesupport-4.0.2/lib/active_support/core_ext/string/indent.rb:41:in `tap'
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/activesupport-4.0.2/lib/active_support/core_ext/string/indent.rb:41:in `indent'
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/cucumber-1.3.10/lib/cucumber/formatter/console.rb:35:in `format_step'
/Users/jason/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/cucumber-1.3.10/lib/cucumber/formatter/pretty.rb:155:in `step_name'

I tried creating a failing feature in the 1.3.x branch, but was unable to get it to fail. I haven't got time to look into this further at the moment. With any luck, someone more familiar with the code will have some idea what's going wrong. I'll try to dig deeper when I get a chance.

I created a branch with a new feature here:

https://github.com/subakva/cucumber/blob/bugs/fix-expanded-pretty-formatter/features/pretty_formatter.feature

@subakva

This comment has been minimized.

Show comment
Hide comment
@subakva

subakva Jan 15, 2014

I just noticed that cucumber doesn't use ActiveSupport for indenting strings. I think the problem is that ActiveSupport chokes on a negative argument to indent, while the cucumber version gives it the old college try. This would explain why the test doesn't fail.

Some options to fix this:

  • account for the expanded names while calculating max_line_length
  • explicitly call the internal #indent method rather than relying on the monkey-patched String method.

The first option will allow the comments to line up as intended. The second option is easier to implement and results in consistent formatting no matter what the monkeys have been patching.

subakva commented Jan 15, 2014

I just noticed that cucumber doesn't use ActiveSupport for indenting strings. I think the problem is that ActiveSupport chokes on a negative argument to indent, while the cucumber version gives it the old college try. This would explain why the test doesn't fail.

Some options to fix this:

  • account for the expanded names while calculating max_line_length
  • explicitly call the internal #indent method rather than relying on the monkey-patched String method.

The first option will allow the comments to line up as intended. The second option is easier to implement and results in consistent formatting no matter what the monkeys have been patching.

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Aug 29, 2014

Member

Could you try again with version 2.0.0.beta.1 please? The code has changed a lot and there's a chance this bug has gone away 😀

Member

mattwynne commented Aug 29, 2014

Could you try again with version 2.0.0.beta.1 please? The code has changed a lot and there's a chance this bug has gone away 😀

@brasmusson

This comment has been minimized.

Show comment
Hide comment
@brasmusson

brasmusson Sep 25, 2014

Contributor

This issue is fixed in v2.0.0.beta.1 (more specifically by cucumber/cucumber@84eb09c). The guard that ensures that the argument to indent is >= 1 is here.
@mattwynne How do you want to close this issue? With a spec and updated History.md (a spec that shows that source strings are not aligned, but there is at least one blank between the step and the source string), or should it be closed right here?

Contributor

brasmusson commented Sep 25, 2014

This issue is fixed in v2.0.0.beta.1 (more specifically by cucumber/cucumber@84eb09c). The guard that ensures that the argument to indent is >= 1 is here.
@mattwynne How do you want to close this issue? With a spec and updated History.md (a spec that shows that source strings are not aligned, but there is at least one blank between the step and the source string), or should it be closed right here?

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Sep 28, 2014

Member

I'm happy just to close it. We can deal with any future issues as they cause people problems.

Member

mattwynne commented Sep 28, 2014

I'm happy just to close it. We can deal with any future issues as they cause people problems.

@mattwynne mattwynne closed this Sep 28, 2014

marxarelli added a commit to marxarelli/cucumber that referenced this issue Oct 26, 2015

Avoid negative `#source_indent` which blows up in certain cases
Some monkey patched implementations of `String#indent`, such as Active
Support's, do not handle negative values (see issue cucumber#620). This behavior
isn't strictly a bug in Cucumber but a lower bound of 0 in
`Cucumber::Ast::HasSteps#source_indent` would avoid these types of
errors from bubbling up.

marxarelli added a commit to marxarelli/cucumber that referenced this issue Oct 26, 2015

Avoid negative `Cucumber::Ast::HasSteps#source_indent`
Some monkey patched implementations of `String#indent`, such as Active
Support's, do not handle negative values (see issue cucumber#620). This behavior
isn't strictly a bug in Cucumber but a lower bound of 0 in
`#source_indent` would mitigate the occurrences of related errors.

mattwynne added a commit that referenced this issue Apr 19, 2016

Avoid negative `Cucumber::Ast::HasSteps#source_indent` (#926)
Some monkey patched implementations of `String#indent`, such as Active
Support's, do not handle negative values (see issue #620). This behavior
isn't strictly a bug in Cucumber but a lower bound of 0 in
`#source_indent` would mitigate the occurrences of related errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment