Get to green, then update dependencies #526
Conversation
|
@etagwerker what do you think of this approach? I think it should unblock us in other areas. |
|
Hey @faisal, this looks really good! |
cucumber-core fails on Ruby 3.5.0dev due to a change in the expected method signature for Cucumber::Core::Test::Location#from_source_location. Work around this by aliasing out the old method and then using a new proxy method to catch the method and send the old style message to the old method.
Also corrected multiple Minitest/EmptyLineBeforeAssertionMethods issues flagged by rubocop-minitst.
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
@faisal One question, how long do you think we should have this monkey patch? Is there any way to have a breaking condition to remember us removing it?
Like onces Cucumber is at version X, raise "we do not need this patch please remove me"
Just wondering 🤔 to don't have unnecessary code in the future
There was a problem hiding this comment.
I wrestled with this question. The initial version of the patch did a version check on Cucumber as well as Ruby, but tests then failed when they updated Cucumber without fixing the issue.
I think our options are:
- Leave it as is for now, but watch updates for a fix. In practice we're already in this situation with Ruby, since the non-dev version of Ruby won't run this workaround.
- Make it specific to Cucumber's current version range, and live with having to update that version range when they continue revving Cucumber without a fix.
In either case, if we merge this I think we should open an issue to revert the workaround at such time when it becomes unnecessary.
There was a problem hiding this comment.
I'm watching for cucumber/cucumber-ruby-core#292 to land, or to be closed out in favor of something equivalent.
There was a problem hiding this comment.
I see. Yeah, I agreed, let's keep it as it is and wait for cucumber/cucumber-ruby-core#292 to be merged, in good news, there is already a PR.
JFI: I'm trying to get someone with powers to merge this ASAP
There was a problem hiding this comment.
One last option would be to rework or replace that feature so as to not call cucumber-core. That seems more involved than is warranted.
There was a problem hiding this comment.
Yeah! that is another great option. do you have an estimation on which tests are using that feature? How long would it take?
I mean, either way we can begin with merging this PR then we can stop using that feature and upgrade cucumber as needed.
| def print_starting_up_output | ||
| puts "\n\n!!! Running `#{name}` rake command\n" | ||
| puts "!!! Inspecting #{paths} #{options.empty? ? '' : "with options #{options}"}\n\n" | ||
| puts "!!! Inspecting #{paths} #{"with options #{options}" unless options.empty?}\n\n" |
There was a problem hiding this comment.
@faisal maybe changing this to single quotes helps the reading? what do you think?
| puts "!!! Inspecting #{paths} #{"with options #{options}" unless options.empty?}\n\n" | |
| puts "!!! Inspecting #{paths} #{'with options #{options}' unless options.empty?}\n\n" |
| spec.add_development_dependency 'mocha', '~> 2.7.1' | ||
| spec.add_development_dependency 'ostruct' | ||
| spec.add_development_dependency 'rake', '~> 13.2.0', '>= 11.0.0' | ||
| spec.add_development_dependency 'rake', '~> 13.3.0', '>= 11.0.0' |
There was a problem hiding this comment.
@faisal one more question: why did you bump the dependencies in the same PR? Was that needed to make CI pass?
There was a problem hiding this comment.
If not, could we split it? Sorry for asking a lot but want to make sure we are not blowing it up
There was a problem hiding this comment.
This PR wound up collapsing a bunch of other PRs: #504, #512, #524.
In particular, FakeFS < 3 absolutely spewed the console with fakefs-2.6.0/lib/fakefs/globber.rb:54: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information) warnings in test runs. The others were a combination of nice-to-have modernization and chicken-and-egg updates. It's not ideal, but it did get us out of a pile of parallel smaller PRs all of which failed. I expected we could have a conversation about it before merging, but since it works I don't think we should revert the merge unless we find specific problems.
| spec.add_dependency 'parser', '>= 3.3.0.5' | ||
| spec.add_dependency 'rainbow', '~> 3.1.1' | ||
| spec.add_dependency 'reek', '~> 6.4.0', '< 7.0' | ||
| spec.add_dependency 'reek', '~> 6.5.0', '< 7.0' |
There was a problem hiding this comment.
This is the most significant change (not a dev dependency): https://github.com/troessner/reek/blob/master/CHANGELOG.md
But based on the reek changelog it should be fine to merge.
|
@faisal thanks for the contribution! please feel free to keep it going; 🎖️ |
3 participants
This update includes two commits to get back to green, and one to update the bundled gems to current versions.
Getting to green:
Updating the bundle:
Check list: