enkessler / cuke_linter

A linting tool for Cucumber
MIT License
32 stars 8 forks source link

TestShouldUseBackgroundLinter False negative when using Scenario Outline #12

Closed thiagotrentin closed 4 years ago

thiagotrentin commented 4 years ago

Hi All,

I guess we're getting a false negative with TestShouldUseBackgroundLinter issue in our feature files with Scenario Outline using values from examples in the first line.

The expected behavior is to warn when this kind of problem occurs.

Feature: TestShouldUseBackgroundLinter Problema Example
  False negative using Scenario Outline

  Scenario Outline: Scenario Outline name
    Given two Scenario Outline's in the same file with <value>
    When running Cuke Linter
    Then don't receive a TestShouldUseBackgroundLinter

    Examples:
      | value  |
      | sample |

  Scenario Outline: Scenario Outline name
    Given two Scenario Outline's in the same file with <value>
    When running Cuke Linter
    Then don't receive a TestShouldUseBackgroundLinter

    Examples:
      | value  |
      | sample |

When running cuke_linter will give you this result:

TestShouldUseBackgroundLinter
  Test shares steps with all other tests in feature. Use a background.
    path/to/file/test_should_use_background_linter.feature:4
    path/to/file/test_should_use_background_linter.feature:13

2 issues found

I'd be glad to help resolving this issue, just want to start a discussion about the best solution.

Thank you!

thiagotrentin commented 4 years ago

This issue also occurs using data table insite the step

Something like this:

  Scenario: Scenario name
    Given two Scenario's in the same file with data
      | value  |
    When running Cuke Linter
    Then don't receive a TestShouldUseBackgroundLinter

    Examples:
      | value  |
      | sample |

  Scenario: Scenario name
    Given two Scenario's in the same file with data
      | value  |
    When running Cuke Linter
    Then don't receive a TestShouldUseBackgroundLinter
enkessler commented 4 years ago

@thiagotrentin Good catch and thanks for the feedback!

Currently, that linter only checks the text of the step. It doesn't know if there is a block attached or if parameter substitution will occur. So that's why it's triggering on your examples.

The linter could be enhanced to take a step's block into account. That'd be as easy as considering the text of the block to be part of the text of the step and then checking for a match normally. The outline cases, on the other hand, might be complex enough that it might be easier to just turn off the linter for those affected outlines.

Given that Cucumber (at least, the current Ruby version that I am familiar with) does not apply outline parameters to background steps, I agree that this linter should not suggest moving such steps to the background. The problem, in that case, is reliably detecting a parameterized step. I think that Cucumber only uses < and > to demarcate parameters but we'd have to scan for those, make sure that we handle them when they are escaped (e.g. \<), check for them in step blocks as well, etc. It should be possible but it also crosses my threshold of "I'll do it myself" and lands in "I'd be happy to pair with you on your first contribution to the project". ;)

enkessler commented 4 years ago

@thiagotrentin I went ahead and started to update the linter so that it considers any doc string or table attached to the step and I found out that the linter wasn't intentionally limiting itself to just the text of the step. It was just using == and it was the step models themselves that were the limiting factor. I think that it's a good idea to keep using == and so the real solution is to update the CukeModeler::Step class so that it includes any attached tables or text blocks when determining equality using ==.

enkessler commented 4 years ago

Good news! cuke_modeler 2.0 is out, which has the updated == and cuke_linter likewise has a new release that allows it to use the new cuke_modeler. This should solve your second case.

thiagotrentin commented 4 years ago

Hi @enkessler ,

What a nice surprise! I just get back from some days off and can validate this right now.

Thank you for your help!

thiagotrentin commented 4 years ago

@enkessler the lastest version almost resolved all my issues, the only one that remains is when we have something like this:

  Scenario Outline: Scenario Outline name
    Given two Scenario Outline's in the same file with "<value>"
    When running Cuke Linter
    Then don't receive a TestShouldUseBackgroundLinter

    Examples:
      | value  |
      | sample |

When I have and datatable value inside double quotes the linter still throwing the TestShouldUseBackgroundLinter.

I need find out if this is a good pratice, but it works with Cucumber. So if this isn't a good pratice we can have another linter or to look for < and > in the whole step. What do you think?

enkessler commented 4 years ago

Double quotes really shouldn't make a difference to the linter or modeler and I'd be concerned if they did.

As far as detecting a parameterized step goes, I took a look at the Gherkin tests and it looks like it doesn't mess with parameter substitution. That means that what counts as a parameter or not is up to Cucumber, which means that it may be implementation specific. For example: in the Ruby version, <param> will get replaced with a value but < param >will not.

The linter does have access to the outline's parameters and the full text of the step. So we could just do a loop over the parameters and check if "<#{whatever_the_paramter_is_called}>" appears in the step. It may not be 100% accurate due to differences across Cucumber implementations but it should be better than what it is doing now.

enkessler commented 4 years ago

@thiagotrentin 0.12.1 is now out with a fix for detecting parameterized steps. Please, try it out and let me know.

thiagotrentin commented 4 years ago

I'll do it right now... thank you!

thiagotrentin commented 4 years ago

It works! We can close this issue. Thanks @enkessler