Fix scenario outline #186

Merged
merged 2 commits into from Sep 7, 2012

5 participants

@begriffs

Don't blow up on cucumber Scenario Outlines when the :use_scenario_name option is enabled.

Use a uniquely named cassette for every example row of a Scenario Outline.

Joe Nelson a... added some commits Jul 26, 2012
Joe Nelson and Stephen Anderson Fix :use_scenario_name behavior for cucumber scenario outlines 5fc309b
Joe Nelson and Stephen Anderson remove debugging artifact c061001
@myronmarston myronmarston commented on the diff Jul 27, 2012
lib/vcr/test_frameworks/cucumber.rb
+ case scenario
+ when Cucumber::Ast::Scenario
+ File.join(scenario.feature.name.split("\n").first, scenario.name)
+ when Cucumber::Ast::ScenarioOutline
+ # This happens if we trigger a Scenario Outline
+ # in isolation. First example row is not accessible.
+ File.join(scenario.feature.name.split("\n").first, scenario.name, 'first_example')
+ when Cucumber::Ast::OutlineTable::ExampleRow
+ # ExampleRow's scenario.name holds the example itself
+ File.join(scenario.scenario_outline.feature.name.split("\n").first, scenario.scenario_outline.name, scenario.name)
+ else
+ raise "Unhandled class: #{scenario.class.name}"
+ end
+ else
+ "cucumber_tags/#{tag_name.gsub(/\A~?@/, '')}"
+ end
@myronmarston
VCR member
myronmarston added a line comment Jul 27, 2012

There's quite a bit of duplication here (e.g. feature.name.split("\n").first shows up 3 times).

Can you pull out some helper methods for this kind of stuff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston commented on the diff Jul 27, 2012
...res/test_frameworks/cucumber_scenario_outline.feature
+ When a request is made to "http://localhost:7777/localhost_request_1"
+ Then the response should be "Hello localhost_request_1"
+ Examples:
+ | key | value |
+ | foo | bar |
+ """
+ And the directory "features/cassettes" does not exist
+ When I run `cucumber WITH_SERVER=true features/vcr_example.feature`
+ Then it should pass with "1 scenario (1 passed)"
+ And the file "features/cassettes/VCR_example/scenario_outline/_foo_bar_.yml" should contain "Hello localhost_request_1"
+
+ # Run again without the server; we'll get the same responses because VCR
+ # will replay the recorded responses.
+ When I run `cucumber features/vcr_example.feature`
+ Then it should pass with "1 scenario (1 passed)"
+ And the file "features/cassettes/VCR_example/scenario_outline/_foo_bar_.yml" should contain "Hello localhost_request_1"
@myronmarston
VCR member
myronmarston added a line comment Jul 27, 2012

I'd like to see some specs for the changes; in my day-to-day TDD process working on VCR, I don't usually run the cukes and I'd like to have some specs fail if I break it here. You can see the current specs for ideas of how to do this:

https://github.com/myronmarston/vcr/blob/master/spec/vcr/test_frameworks/cucumber_spec.rb

On top of that, I don't really think of this as a separate feature that warrats its own cucumber feature file. This doesn't add any documentation value to what's published to relish. Instead, I'd like to see changes to the existing cuke to exercise this functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@johnhampton

This functionality is very useful. Will it be pulled?

@myronmarston
VCR member

This functionality is very useful. Will it be pulled?

Yep, it'll get pulled in at some point. I had some concerns about the PR as it exists (see my comments above), and I like to give contributors the chance to fix up PRs themselves rather than just doing it for them. However, given that I've been waiting on a response form @begriffs for a month, it seems like I'll need to address the issues myself. I have less time these days (my 2nd child was just born) but hope to get to it at some point.

@Jacobkg could you take a look at fixing up and merging this PR (and the other one) the next time you're working on VCR?

@begriffs

Sorry @myronmarston, I forgot about the issue. I'll take a another look at it and submit an updated PR later this week.

@myronmarston
VCR member

Sorry @myronmarston, I forgot about the issue. I'll take a another look at it and submit an updated PR later this week.

No worries. I made some comments on your other PR (#185) as well -- can you take a look at it, too?

@harryw

+1 from me too - I'd really like the Cucumber tag feature to work with scenario outlines.

@begriffs

Hmm, I don't know how to properly mock instances of Cucumber::Ast::Scenario, Cucumber::Ast::ScenarioOutline, and Cucumber::Ast::OutlineTable::ExampleRow in rspec. The cucumber feature in this PR is certainly overkill, but I can't see how else to exercise the various situations that it tests.

@myronmarston
VCR member

Hmm, I don't know how to properly mock instances of Cucumber::Ast::Scenario, Cucumber::Ast::ScenarioOutline, and Cucumber::Ast::OutlineTable::ExampleRow in rspec. The cucumber feature in this PR is certainly overkill, but I can't see how else to exercise the various situations that it tests.

Check out the existing specs for the cucumber support. As you can see, we are testing the hooks in isolation from cucumber (since there's not a good way that I know of to load cucumber without running it in an rspec context). The scenario object is just a simple mock object that has the same nested attribute interface that a real cucumber scenario has. This isn't a perfect solution by any means (if cucumber changes the interface of the scenario object, this test could continue to pass even if the integration is broken), but it does give us good regression coverage so that we can refactor the cucumber integration code and have fast specs that give us feedback about if it's still interacting with the cucumber interface the same.

We also have the cucumber cucumber feature (yes, it's a bit meta) that exercises the same code against real cucumber. The cukes function as documentation and help us ensure things continue to actually work, but I don't usually run them locally because they're slow -- which is why I want the spec coverage as well (which I do run locally when I'm developing, all the time).

So my suggestion is to follow a similar approach to what's already there:

  • Use a nested stub to create mock versions of the various cucumber objects for the unit tests.
  • Update the existing cucumber cucumber feature so that it also involves all these different cucumber objects.

That'll give us the best of both worlds, I think, without the overkill of an additional cucumber feature that doesn't serve any sort of documentation purpose.

@begriffs

Thanks for your notes, but I realize I don't have the time to fix this PR. I'm leaving it for someone else.

@Jacobkg
VCR member

I'm gonna merge the pull request, and then make some of the changes Myron suggested.

@Jacobkg Jacobkg merged commit a39a8e3 into vcr:master Sep 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment