-
Notifications
You must be signed in to change notification settings - Fork 228
[CHANGE] Bump cucumber dependency #531
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, and I got the green.
This does not fix the situation for our case.
Also, I double-check this, and you are right :/.
rubycritic.gemspec
Outdated
| end | ||
| spec.add_development_dependency 'cucumber', '~> 10.0.0', '!= 9.0.0' | ||
| spec.add_development_dependency 'cucumber', '~> 10.1.0', '!= 9.0.0' | ||
| spec.add_development_dependency 'cucumber-core', '>= 15.2.1', '~> 15.2.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@faisal @julioalucero sorry, I do not get it... what is the issue here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to issue #530? If so, I can link it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JuanVqz yes it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is just to bump the cucumber version and specify a cucumber-core version.
In theory, the cucumber update was going to prevent the issue that necessitated the workaround that I intend to remove with #530, but it does not appear to do so.
If you have a fix for the underlying issue for which I made the workaround that #530 removes, that'd be very useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about it, the more I think I should remove the explicit cucumber-core requirement. Before I push an update, does anyone want to argue to keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a fix for the underlying issue for which I made the workaround that #530 removes, that'd be very useful.
I think a good solution would be to stop using the functionality in cucumber we use in our test suite, I think you suggested that in a previous PR, I wonder how many tests would be using that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it is not really needed to remove the monkey patch right away, it is fine just having the issue and eventually when the cucumber gem releases a new version that allow us remove the monkey patch just remove it, so, no worries a lot about it if there is no direct fix, you are welcome to contribute in something else in the meantime @faisal 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'm pushing a simplified commit that only updates the cucumber dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is fun: I went to test the monkeypatchless version (to answer the question of what tests hit the changed method signature issue) against a current Ruby 3.5.0 dev build, and...
Could not find compatible versions
Because aruba >= 1.1.2 depends on bundler >= 1.17, < 3.0
and the current Bundler version (4.0.0.dev) does not satisfy bundler >= 1.17,
< 3.0,
aruba >= 1.1.2 cannot be used.
So, because Gemfile depends on aruba >= 2.3.1, < 2.4.A,
version solving has failed.
Your bundle requires a different version of Bundler than the one you're running.
Install the necessary version with `gem install bundler:2.7.2` and rerun bundler
using `bundle _2.7.2_ update`
That's largely out of our control, so I guess it's time to turn my attention back to #508.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking it! yeah, no worries about it for now but don't forget about it :)
The simplified version looks good for now, let me approve it.
Update to latest cucumber # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! ![]()
Update to cucumber dependency.
Check list: