Updates for frozen-string-literal compatibility. #2437
Conversation
|
Oh, one more thing: Travis should fail on this PR because I've made changes to get things working with the latest CodeRay release/commits, and their colours have changed after v1.0.9 (which is what you're locked to). |
In general, there's no good reason for us to remain on an old version of cucumber. We just haven't noticed the new releases or had anything forcing us to upgrade, so no one has put any effort into upgrading cucumber. That said, RSpec still supports Ruby 1.8.7 and 1.9.2 (and, according to our understanding of SemVer, must continue to support it until RSpec 4), and it looks like cucumber 2.x won't install on those versions. So I think our options are:
I think any of those are viable options, and don't particularly feel strongly about it, as long as the maintenance burden is low (which might eliminate the 2nd option). What do others think? /cc @rspec/rspec |
lib/rspec/core/notifications.rb
Outdated
| "\n" << colorizer.wrap(" # #{formatted_caller}\n", :detail) | ||
| colorizer.wrap("\n #{pending_number}) #{example.full_description}", :pending) + "\n " + | ||
| Formatters::ExceptionPresenter::PENDING_DETAIL_FORMATTER.call(example, colorizer) + | ||
| "\n" + colorizer.wrap(" # #{formatted_caller}\n", :detail) |
There was a problem hiding this comment.
This change is going to allocate lots of strings, as each time a + b is executed, a new string is allocated. Can we make the initial string mutable instead?
There was a problem hiding this comment.
Opted to wrap all the strings in an array and then call join - felt a bit neater than making just one string mutable. Happy to change it though.
|
So, I just spent some time getting edge cucumber and edge aruba working with the rspec-core test suite. There was one minor change related to frozen string literals, and I've pushed that commit. I've also submitted related PRs:
It's important to note, though: Cucumber uses an old version of Aruba, and so to get frozen-string-literal-friendly versions of both introduces a whole bunch of upgrading challenges (and if you want to support older Ruby versions while using multiple versions of Cucumber - the second option suggested - I foresee some serious headaches). Hence, my vote - as much as it counts, given I'm not a maintainer here despite recent suggestions/warnings from some of you ;) - is for option 3 - though granted, that's still reliant on the Cucumber team updating their use of Aruba. An alternative option: run the cucumber tests only on older Ruby versions? |
I don't understand what you mean by this. I can see that Cucumber uses Aruba In what way is cucumber's use of an old version of Aruba causing us problems? On a side note: it would be nice to get this green so we can merge. Can you undo the coderay-related changes or also include an upgrade to coderay in this PR so we can get the build green? |
I don't think we should seriously consider this. We're going to drop older ruby versions eventually (RSpec 4 will definitely drop support for Ruby 1.8.7, and possibly Ruby 1.9.x) and I don't want us to be in the position where the only versions on which we run certain automated checks are older ruby versions. |
Using the latest released version, which will at least pass with mutable string literals.
|
Very valid point on when to run the cucumber tests - and I'd missed that Aruba is only a development dependency of Cucumber, so I can work on a separate PR that upgrades the RSpec Aruba usage if you'd like? Also, just pushed a commit to use CodeRay 1.1.1, which should be green in CI with mutable string literals. |
Yes, I'd like :).
Thanks. The JRuby build failed due to some timeouts (it's an on-going intermittent problem, sadly) but I restarted those parts of the build so hopefully it'll go green this time. |
This gets both specs and features passing without deprecation warnings using Aruba 0.14.2, as discussed in rspec#2437. Two caveats: * There is a 1.0.0 pre-release version of Aruba, but it requires Cucumber 2, and that’s a whole different can of worms to open. * The gemspec notes that Aruba 0.7 is broken on MRI 1.8.7. I’m not sure if 0.14 is happier, but perhaps Travis will tell us. If it’s not, I guess this change can’t be merged in until RSpec 4 is on the cards?
I don't think this is correct, due to http://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api. |
|
@mvz there's not right or wrong, just our interpretation :) We've consistently held that we were going to support 1.8.7 through to RSpec 4, since it would be a disruptive, breaking change to a large part of our user base. This is in our view aligned with the spirit of semver. |
|
@xaviershay that makes sense. Thanks for explaining. |
This gets both specs and features passing without deprecation warnings using Aruba 0.14.2, as discussed in rspec#2437. Two caveats: * There is a 1.0.0 pre-release version of Aruba, but it requires Cucumber 2, and that’s a whole different can of worms to open. * The gemspec notes that Aruba 0.7 is broken on MRI 1.8.7. I’m not sure if 0.14 is happier, but perhaps Travis will tell us. If it’s not, I guess this change can’t be merged in until RSpec 4 is on the cards?
Updates for frozen-string-literal compatibility.
(I've got patches for
rspec-expectations,rspec-mocksandrspec-supportwhich are related to this one, so I'm going to write out all the detail here, and then link to it from the others.)I've put together these patches by running the test suites for each gem with the
RUBYOPT=--enable-frozen-string-literalflag, so they're more thorough than my previous patches on this topic. However, as noted in those previous discussions, the test suite cannot yet be run reliably in that situation, so I've had some local changes to my Gemfile, and worked on patches for some dependencies. I've also had to have all rspec-x repos locally to debug some of the fixes in these patches.Also: I've not got the cucumber test suites working in
rspec-core,rspec-expectations, orrspec-mocks, so there could very well be further places in the Spec codebase that need changing. You're using a two-year-old version of Cucumber (1.3.x, when 2.x is released and 3.x is on the way), so I'm not sure how much effort should be put into updating that? Should we fork the 1.3.x release and make that frozen-string-literal-safe? Although, there are issues in Cucumber's dependencies as well…Regarding how I've patched things: generally, I'm using
dupinstead ofString.new- this is mostly because I've been working on similar patches for Rails, and the maintainers there preferdup, hence that's my current habit. Also,String.newhas an ASCII encoding, whereasString.new('some string')respects the argument's encoding (which defaults to UTF in recent Rubies), and that inconsistency is annoying.I could (and might) keep going down the dependency rabbit hole and offer further patches, but I wanted to at least get these changes submitted so they're useful for the RSpec team sooner rather than later.
The dependency situation as it currently stands:
Patched (albeit with no new versions yet):
Pull Requests submitted:
In-progress patches:
No known patches:
For anyone wanting to help: if you have commit bits on any of the above dependency repos, maybe get outstanding PRs merged, or help investigate failing test suites (racc). If anyone's on the Cucumber team: discussions need to be had around updating the version of Aruba it's dependant on, and maybe migrating from Syntax to Coderay (as that's what's recommended in the Syntax README).