cucumber / godog

Cucumber for golang
MIT License
2.22k stars 250 forks source link

godog.Steps not defined #81

Closed agajdosi closed 7 years ago

agajdosi commented 7 years ago

Hi, I am having problem with nested steps. My context is following: I want to use custom dynamic variables inside of feature file, for example: Then body of http request on "${server_ip}/health" is "ok". The thing is that ${server_ip} is changing on each start of test.

To handle this I wanted to use nesting of steps:

s.Step(`step:"minishift ip" should return same ip`, m.shouldReturnSameIp)
s.Step(`(.*)`, injectVar)

func injectVar(stepName string) golang.Steps {
   [supplementing ${server_ip} for actual IP of server in stepName string]
   return golang.Steps{
     fmt.Sprintf(`step:%s`, stepName),
   }
}

But I am still getting: undefined: golang in golang.Steps. Am I wrong somewhere, or missing something?

l3pp4rd commented 7 years ago

unless you named godog imported package as golang, godog.Steps are available in recent version. Try to update the package go get -u github.com/DATA-DOG/godog/cmd/godog.

l3pp4rd commented 7 years ago

Though, your returned step in godog.Steps will run recursively forever. because it always will match (.*) step.

s.Step(`step:"([^"]*)" should return same ip`, m.shouldReturnSameIp)
s.Step(`(.*)`, injectVar)

func injectVar(stepName string) godog.Steps {
   [supplementing ${server_ip} for actual IP of server in stepName string]
   return godog.Steps{
     fmt.Sprintf(`step:"%s" should return same ip`, stepName),
   }
}

Should be something like that.. But it looks quite misleading to make such dynamic implementations in step level. I do not consider this as a good practice

agajdosi commented 7 years ago

@l3pp4rd Thank you for reply. I think it should take first Step which regex it matches, which will be m.shouldReturnSameIp, not continuing further - so no recursive loop, is it right?

I do not consider this as a good practice

Do you have any suggestion how to implement dynamic variables the better/correct way? I would love to have this as part of cucumber, but unfortunately I have not found anything on this topic...

l3pp4rd commented 7 years ago

Since the server IP is dynamically changing, you should never directly include it in features. See the api example.

Scenario: does not allow POST method
    When I send "POST" request to "/version"
    Then the response code should be 405
    And the response should match json:
      """
      {
        "error": "Method not allowed"
      }
      """

You should use only path in request uri. The step definition should automatically take the correct IP address, which is changed in your case before every scenario as far as I understand, and then perform the request. Because you aren't testing the server IP address changes, you are testing the API of your application right?

You should make your steps orthogonal and composable, your context is what your application does, not how it does it. Steps should not hint what kind of server address is it using, or what kind of database is used. The steps should represent the things your users are exposed to - in other words the public interface of your application.

agajdosi commented 7 years ago

@l3pp4rd Found out I reverted changes in glide.yaml by mistake so godog was 0.6.2 not 0.7.1 as I thought. All working now.

Thank you for your insight and advice, will consult these information with team. Feel free to close this, all questions answered I think. Thanks.

hferentschik commented 7 years ago

@l3pp4rd

Should be something like that.. But it looks quite misleading to make such dynamic implementations in step level. I do not consider this as a good practice

I agree, it is not ideal and if it were just about checking the API of an web app, I'd agree with you that we should only use the path. However, we are actually testing a CLI (see https://github.com/minishift/minishift/blob/master/test/integration/features/basic.feature) and the dynamic IP and hostname are part of the API. There seems no easy way to handle this. The idea is to have a set of known "variables" (IP, hostname, etc) which can be used for various steps.

@l3pp4rd, in this case, what you think about the idea/approach? Do you have a better idea?

l3pp4rd commented 7 years ago

Hi, well fine. I see that your feature starts a VM and it depends on the state of previous scenario, since this is a costly operation it would certainly be slow to reset completely all the state and start VM on every scenario. Though, that would give you an option to run scenarios concurrently.

On the other hand, since starting a VM is a precondition to all of the tests, that could be in TestMain directly, and that would allow you to run scenarios concurrently if you cleanup the VM internal state before every scenario. In that case, that would be similar to the database, you start it before running all your scenarios.

Anyway, concerning IP. As soon as you start the VM it is known and could be stored in suite context and though, reused over steps.

I may misunderstand something, but it seems like the IP depends on the VM started.

hferentschik commented 7 years ago

On the other hand, since starting a VM is a precondition to all of the tests, that could be in TestMain directly, and that would allow you to run scenarios concurrently if you cleanup the VM internal state before every scenario. In that case, that would be similar to the database, you start it before running all your scenarios.

This is partly true. Some scenarios could indeed be parallelized this way. On the other hand the start of the VM itself can take different parameters which needs to be tested as well. The current feature file is just the beginning.

Anyway, concerning IP. As soon as you start the VM it is known and could be stored in suite context and though, reused over steps.

Right, once the VM is started, the dynamic parameters are known and they should be stored in a suite context. We agree on this for sure.

The question is now how you write the tests. Take minishift ip which returns the IP of the VM. Using variables you could write:

 When executing "minishift ip" succeeds
 Then stdout should contain
  """
  #{vm_ip}
  """

You can reuse a lot of the general step definitions we are using already. Same for testing URLs, etc.

Without variables, I need to write a dedicated step:

 When executing "minishift ip" succeeds
 Then stdout contains the IP of the VM

You need this custom steps everywhere where the dynamic IP plays a role. The thought was to avoid this by the variable mechanism, reducing the amount of steps one needs to implements.

Either way, at some stage the information about the current IP needs to be retrieved from the context at some stage and matched.

If I understand you correctly, you prefer the dedicated step approach.

l3pp4rd commented 7 years ago

In this case, a dedicated step makes more sense, but if you have more similar features, where you need to use dynamically generated values, there is a different approach. Like for instance there are cases where inserted records in database, produce auto incremented primary key and you cannot guess what it is.

You can create a placeholder value map. And whenever a scenario produces dynamic value, you add a placeholder for this value, like suite.placeholders["VM_IP"] = parsedIP. Then all the output and URLs you use in step variables, can search and replace these placeholders. And in result you have something like this:

Scenario: User is able to retrieve host and port of OpenShift registry
    Given Minishift has state "Running"
     When executing "minishift openshift registry" succeeds
     Then stdout should contain
      """
      VM_IP:5000
      """

  Scenario: access health endpoint
     Given Minishift has state "Running"
     When I send "GET" request to "http://VM_IP:5000/health"
     Then the response should contain json:
      """
      {
         "ip": "VM_IP",
         "status": "UP"
      }
      """

In general the placeholder matching is based on your preference. But note, that this is harder to understand for the user. Steps like Given Minishift has state "Running" could already take and store this IP for the scope of scenario, and validate that the IP is produced.

Because, this reads better:

Scenario: access health endpoint
     Given Minishift has state "Running"
     When I send "GET" request to "/health"
     Then the response should contain json:
      """
      {
         "status": "UP"
      }
      """
hferentschik commented 7 years ago

In this case, a dedicated step makes more sense,

Ok

but if you have more similar features, where you need to use dynamically generated values, there is a different approach You can create a placeholder value map.

This is exactly what we referred to as "variable substitution" and which was described in the initial issue (using nested steps was just an idea on how to implement it). The only difference is ,that we had some sort of Ruby like variable syntax (#{VM_IP}) in mind, whereas you just use VM_IP (without # and curly braces). Looking at your example I think I prefer your convention now.

Then all the output and URLs you use in step variables, can search and replace these placeholders

+1 Right, that was the idea.

In general the placeholder matching is based on your preference. But note, that this is harder to understand for the user.

Agreed, it is a tradeoff of sorts.

@l3pp4rd, thanks a lot for all this feedback. Much appreciated. It feels good to get our ideas validated by someone with more Cucumber experience.