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

support cucumber 1.1.5 #644

Closed
wants to merge 1 commit into from
Closed

support cucumber 1.1.5 #644

wants to merge 1 commit into from

Conversation

e2
Copy link
Contributor

@e2 e2 commented Feb 18, 2012

Cucumber 1.1.5 renames a few things, notably:

source_tag_names

to

source_tags

The patch allows Capybara to handle both versions.

Cheers,

Cezary

@rtlong
Copy link

rtlong commented Feb 18, 2012

The corresponding issue on Cucumber: cucumber/common#227

@joliss
Copy link
Collaborator

joliss commented Feb 18, 2012

Cc: @aslakhellesoy

Wait, how come stuff is breaking for us at a patch-level release? Is source_tags not part of Cucumber's public API? Should we be using something different?

@aslakhellesoy
Copy link
Contributor

My bad, I would have bumped minor if I realised someone else used this API. It didn't occur to me that Capybara would.

This fix is broken though, source_tags returns an Array of Tag, so a #map is needed.

@joliss
Copy link
Collaborator

joliss commented Feb 18, 2012

My bad, I would have bumped minor if I realised someone else used this API. It didn't occur to me that Capybara would.

Haha, oh well. :-)

I wonder, rather than breaking things for users of Capybara 1.1.2 and adding ugly responds_to hackery to Capybara 1.1.3, perhaps it's easier to re-add the source_tag_names (either using .map or using the original implementation) to Cucumber, and push Cucumber 1.1.6 this weekend? Aslak, what are your thoughts?

Thanks for all your hard work on Cucumber, by the way!

@joliss
Copy link
Collaborator

joliss commented Feb 18, 2012

By the way, for anyone looking to test Capybara's Cucumber integration: There is a Cucumber feature suite in Capybara to exercise it ("rake cucumber").

@doolin
Copy link

doolin commented Feb 18, 2012

I missed this and submitted a not-as-good pull request, which I'll go ahead and close in favor of this one (which is better).

@jnicklas
Copy link
Collaborator

I agree with @joliss, it seems very hack-ish for us to add some kind of workaround for this, any chance cucumber can bring back the source_tag_names method, @aslakhellesoy?

@e2
Copy link
Contributor Author

e2 commented Feb 18, 2012

Hi everyone!

Sorry about the sloppy, "bogus" fix. I just got hit by the wrong set of gems and I was in a real, hurry.

@aslak - yes, I read the code wrong and missed the tags - I thought that if I got it wrong, it would break.

I didn't see a specific test in capybara/features, so I left that till later (to avoid pulling the older gems for testing).

Basically, I just wanted to use a pull request as a quick workaround/bug report for a discussion - until I get more time to look into the problem.

Sorry about that.

@e2
Copy link
Contributor Author

e2 commented Feb 18, 2012

I fixed it: #647

The hack can be removed if Cucumber 1.1.6 renders it unnecessary and 1.1.5 can be marked as a conflict.

But since the damage is already done ...

@e2 e2 closed this Feb 18, 2012
@mattwynne
Copy link

I've pushed a commit to Cucumber that adds #source_tag_names back, mapping over the tags to return their #name.

I can't work out how to test it (good old gnarly Cucumber codebase!) and only have a few minutes, so I'm flying blind. Can someone please either just sanity check or ideally manually test the patch?

cucumber/common@844fefe

@joliss
Copy link
Collaborator

joliss commented Feb 18, 2012

Thanks Matt! I tested it in Capybara like so

diff --git a/Gemfile b/Gemfile
index 07a1f43..32116d9 100644
--- a/Gemfile
+++ b/Gemfile
@@ -5,3 +5,6 @@ gemspec

 @dependencies.delete_if {|d| d.name == "xpath" }
 gem 'xpath', :path => 'xpath'
+
+@dependencies.delete_if { |d| d.name == "cucumber" }
+gem 'cucumber', :path => '~/src/cucumber'

and "rake cucumber" now doesn't choke on source_tags, but instead reports that the driver switching isn't working anymore:

expected: :selenium
     got: :rack_test (using ==)
...
features/capybara.feature:23:in `Then Capybara should use the "selenium" driver'

Hm, too bad. I'm not sure off the top of my head what's going wrong. Perhaps it's the updated Gherkin, even.

I tested b7d7e97 (v1.1.15^) though, and it works fine. May I suggest you revert to b7d7e97 and push v1.1.6 with that?

Then we can re-try adding 4bc5ed6 ("Upgraded to gherkin 2.3.8") with more time to figure out what's going wrong.

Btw, re 844fefea ("Add back Scenario#source_tag_names for #227"), I wonder if source_tag_names really has to be deprecated. Semver says that you shouldn't deprecate API without bumping the minor, but even when you go to 1.2.0, is there any need to deprecate it? Capybara for instance wouldn't be able avoid those deprecations without bumping its Cucumber dependency, so they'll just clutter the terminal and annoy users.

@e2
Copy link
Contributor Author

e2 commented Feb 19, 2012

expected: :selenium
got: :rack_test

It probably means instead of a string (tag -> "@Selenium") it got a Tag instance (<....::Tag, @name="@Selenium">) dump as a string, so it defaulted to :rack_test.

In short, the test is using scenarios.source_tags instead of scenarios.source_tags.map(&:name).

It's strange you are getting this, @joliss. I would have expected you to get the same error as here: cucumber/common#229

P.S. With all the chaos around this, I believe @aslak will never again change anything in Cucumber without deprecation ;) Cucumber is just so cool, that people are using every dark corner of Cucumber for whatever they need. And the noise also points out how crucial Capybara is for everyday life.

@mattwynne
Copy link

On 18 Feb 2012, at 23:52, Jo Liss wrote:

Thanks Matt! I tested it in Capybara like so

diff --git a/Gemfile b/Gemfile
index 07a1f43..32116d9 100644
--- a/Gemfile
+++ b/Gemfile
@@ -5,3 +5,6 @@ gemspec

@dependencies.delete_if {|d| d.name == "xpath" }
gem 'xpath', :path => 'xpath'

+@dependencies.delete_if { |d| d.name == "cucumber" }
+gem 'cucumber', :path => '~/src/cucumber'

and "rake cucumber" now doesn't choke on source_tags, but instead reports that the driver switching isn't working anymore:

expected: :selenium
got: :rack_test (using ==)
...
features/capybara.feature:23:in `Then Capybara should use the "selenium" driver'

Hm, too bad. I'm not sure off the top of my head what's going wrong. Perhaps it's the updated Gherkin, even.

Thanks @joliss. Could you re-try this with the current HEAD of Cucumber? I just ran Capybara's features against that and they all passed.

There also weren't any deprecation warnings! Perhaps the pixies fixed them?

I tested b7d7e97 (v1.1.15^) though, and it works fine. May I suggest you revert to b7d7e97 and push v1.1.6 with that?

Then we can re-try adding 4bc5ed6 ("Upgraded to gherkin 2.3.8") with more time to figure out what's going wrong.

You're quite right, that's what we should have done in the first place. If we haven't got this straightened out before Monday morning that's what we'll do.

Btw, re 844fefea ("Add back Scenario#source_tag_names for #227"), I wonder if source_tag_names really has to be deprecated. Semver says that you shouldn't deprecate API without bumping the minor, but even when you go to 1.2.0, is there any need to deprecate it? Capybara for instance wouldn't be able avoid those deprecations without bumping its Cucumber dependency, so they'll just clutter the terminal and annoy users.

Hmm yes. My immediate thought is - "but then how do we remove this kind of clutter from Cucumber?" but of course otherwise we're expecting you guys to have to put backwards-compatiblity hacks like #647 in. I don't mind taking the deprecation warnings out - we can just flag that method up as something we remove in the next major release, I suppose.

@e2
Copy link
Contributor Author

e2 commented Feb 19, 2012

@mattwynne the missing deprecation in outline_table.rb (my fault!) was added after 1.1.7

So most capybara users won't have deprecation warnings until the next Cucumber version bump.

Also, with such mature and popular "gems" (no pun intended!) as Cucumber and Capybara, it might make sense to have versioned API layers in Cucumber - and deprecate an api version as a whole.

Deprecation warnings would show on startup, while allowing you and @aslak to clean up the api as much as you please - and api users would choose if they want a "stable/versioned api" or "development" api.

@joliss
Copy link
Collaborator

joliss commented Feb 19, 2012

Tech first:

Matt wrote:

Thanks @joliss. Could you re-try this with the current HEAD of Cucumber?

Yup, it works great with 1.1.7, and with HEAD 5731b64, it passes but throws deprecation warnings.

I'm not sure that source_tag_names is bad enough to be deprecation-worthy with a minor bump (I'd perhaps wait till version 2.0), but I shall not bike-shed -- it's you guys's call.

@e2 wrote:

In short, the test is using scenarios.source_tags

(In case this still matters: I'm pretty sure I ran the test on a pristine Capybara HEAD, which uses source_tag_names, though I don't know what actually caused the issue.)

@joliss
Copy link
Collaborator

joliss commented Feb 19, 2012

Talk second:

@e2 wrote:

P.S. With all the chaos around this, I believe @aslak will never again change anything in Cucumber without deprecation ;)

Haha, yeah -- I'm glad I got to make this experience vicariously. ^_^

Cucumber is just so cool, that people are using every dark corner of Cucumber for whatever they need. And the noise also points out how crucial Capybara is for everyday life.

Yup. Funny how reports about the breakage started coming in within hours of the release.

@mattwynne wrote:

My immediate thought is - "but then how do we remove this kind of clutter from Cucumber?" but of course otherwise we're expecting you guys to have to put backwards-compatiblity hacks like #647 in. I don't mind taking the deprecation warnings out - we can just flag that method up as something we remove in the next major release, I suppose.

I agree -- I guess it's one of these unpleasant facts of open-source library maintenance that you only get to remove clutter something like once a year (at best).

@e2 wrote:

it might make sense to have versioned API layers in Cucumber - and deprecate an api version as a whole.

I'm thinking that might actually be overkill -- I'm personally not that concerned about breaking things (within reason) between majors, dropping compatibility entirely.


What I think this mess demonstrates is this: When we publish libraries, it really pays to be careful about deciding and communicating what is and isn't public API, and when we use other people's libraries, we should read up on the API documentation (or pester the developers) so we don't accidentally use private methods. Just as a general thought.

Anyways, glad we got this sorted out! Thanks for spending your weekend time. :-)

@e2
Copy link
Contributor Author

e2 commented Feb 19, 2012

@joliss Actually, I take back what I said about the API.

Capybara is a runtime dependency of cucumber-rails:

https://github.com/cucumber/cucumber-rails/blob/master/cucumber-rails.gemspec

Which means every Cucumber change should be tested against Capybara, to make sure cucumber-rails doesn't end up "broken".

Also, cucumber-rails is a good point to handle version conflict information between Capybara and Cucumber.

I'm not sure cucumber-rails can actually work without Rails itself (runtime), but it may be the best solution to give maintainers more development flexibility, while providing the stability through versioning in cucumber-rails.

Because of that, I was wondering if cucumber-rails could be renamed to cucumber-web to handle things like sinatra, etc.

@mattwynne
Copy link

On 19 Feb 2012, at 16:09, Jo Liss wrote:

What I think this mess demonstrates is this: When we publish libraries, it really pays to be careful about deciding and communicating what is and isn't public API, and when we use other people's libraries, we should read up on the API documentation (or pester the developers) so we don't accidentally use private methods. Just as a general thought.

+1. We've put very little thought or effort into this with Cucumber, as yet. Aslak's surprise that Capyara would be depending on this method demonstrates that.

Anyways, glad we got this sorted out! Thanks for spending your weekend time. :-)

No worries. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants