MRI 2.2.2 fixes bug, can test #method again

[jrochkind]

Fixes #285

[michaelherold]

Very cool! Thanks for looking into this; I haven't had the chance to yet.

@dblock How do you think you want to handle this? If we revert that change to the tests, we cannot include 2.2.0 and 2.2.1 in the Travis test suite unless we add them as allowed failures. That's not a useful thing to do though, since the allowed failure could be for something unrelated that only fails on 2.2.0/2.2.1 but is in the added functionality.

Here's the options I see:

1. Don't revert the change and leave the specs as is. This means we can have all versions in the spec suite without issue.
2. Revert the change and add 2.2.0 and 2.2.1 as allowed failures, with the understanding that anyone who merges a PR checks their results in Travis before merging. This adds more work for us when merging things, but overall it's a small amount of work.
3. Revert the change and don't add 2.2.0 and 2.2.1 as allowed failures. When we go to cut a new version of Hashie, we can test to make sure all the tests run on both 2.2.0 and 2.2.1. This is a manual step that isn't too bad if whichever maintainer has a Ruby version manager but is more work than option 2.
4. Cut support for 2.2.0 and 2.2.1 in the gemspec. Since the only issue we're seeing is in the spec suite, this seems a little drastic but it's an option. It's a big decision though, because I believe we should bump the major version if we decide to go this route.

[jrochkind]

Personally, I think it's pretty safe to include 2.2.0 and 2.2.1 as supported versions, without actually testing them in travis. The chances of a failure in one of them that isn't in 2.2.2 is pretty small.

Most projects do not test every possible release. You don't either. You're currently testing 2.1.0 and 2.1.2, but not 2.1.1.

You are currently testing 1.9.3, which means "latest 1.9.3", that is 1.9.3-p550. However, there were ~a dozen different 1.9.3 releases, you are not testing 1.9.3-p125, 1.9.3-p194, etc. You are only testing latest 1.9.3. Similarly, you are only testing 2.0.0, meaning 2.0.0-p633, you are not testing 2.0.0-p195, 2.0.0-p247 etc.

Ruby team finally changed their versioning policy to not use those "pX" suffixes anymore, 2.2.2 is basically the equivalent of what they would have used to call "2.2.0-pSomething", before they stopped using "-pX" suffices. If you don't need to test every pX of 1.9.3 or 2.0.0, then maybe you don't need to test every point release of 2.2.x either?

People should be running latest 2.2.x, just like they should have been running latest patch release of 1.9.3 or 2.0.0 (doesn't mean they do, of course).

[jrochkind]

Technically, if the tests define the supported behavior, then by removing the method() test, it's like saying that being able to reliably call method used to be supported behavior in a previous version of hashie, but isn't anymore. Which technically, seems like you shouldn't drop support for behavior without a major version bump (semver).

Granted, this is behavior that possibly nobody was relying upon, and that should work anyway (modulo ruby bug in 2.2.0 effecting certain circumstances). But tests are of course how you ensure it really is working, not just that it should work. Dropping a test because you know it doesn't pass anymore, without a major version bump, seems poor form.

[jrochkind]

Another possibility, that you might like the best, if you really do want to test in 2.2.0 and 2.2.1 (I'll leave it to you if that means you want to add travis for every single -pX release of 2.0.0 too, haha):

Isolate the problematic test in it's own example. And conditionally mark it pending in rspec, depending on ruby version number.

it "responds to method" do
   if version_is_mri_2.2.0_or_2.2.1 # this is actually the hard part, figuring this out reliably
       skip "Bug in MRI 2.2.0 and 2.2.1 means this behavior is broken in those"
   end
   actual_test_code
end

The hard part is reliably figuring out 'is MRI' from RUBY_PLATFORM, and comparing versions from parsing RUBY_VERSION etc., but people do it.

[michaelherold]

Which technically, seems like you shouldn't drop support for behavior without a major version bump (semver).

I don't feel that the test was really intended to ensure that #method works in that context. I think it was just a brevity thing. That's how it reads to me, anyway. The way the test was written was relying on an understanding of how Ruby itself works, which is why it elicited the bug in Ruby that it did.

Also, I've looked into what was backported into 2.2.2. The test coverage -- and associated fix -- that is present in trunk is not present in the ruby_2_2 branch. Interestingly, the tests do pass on 2.2.2, which indicates that there's more than one moving part in the proc.c code related to using #method with #respond_to_missing? and #method_missing.

[michaelherold]

Isolate the problematic test in it's own example. And conditionally mark it pending in rspec, depending on ruby version number.

I had actually thought of this, but was interrupted while I was writing up my suggestions. Thanks for suggesting it! 👍

This is actually my preferred outcome.

[jrochkind]

Weirdly, there seem to be no changes to proc.c between 2.2.1 and 2.2.2. I can't see any changes that seem even possibly related if this is accurate. Who the heck knows. Argh.

[dblock]

I like where this thread is going, and I am going to delegate any decisions around it to @michaelherold.

[michaelherold]

I think the optimal solution is to leave the tests in, disable them for 2.2.0 and 2.2.1, and add 2.2.2 to the Travis configuration. This means that we can have the most recent set of Rubies tested in CI, since it's likely that they'll be used by people in production for a while.

@jrochkind Is figuring out that version detection something you're interested in doing? If not, I'll look into it later in the week.

[jrochkind]

I've tried to do it before, and it's kind of a mess. There's no good way to tell it's really MRI, except that it's NOT jruby, rbx, or anything else you know about that's not MRI.

Telling the difference between 2.2.0 and 2.2.2 is more straightforward, although annoying, but it's detecting "this is MRI" that I was never able to figure anything good out for.

[michaelherold]

I looked into it for a minute and wanted to drop this here for later reference: ruby_engine seems to handle all the logic for detecting engine/version in a little self-contained class. We could use it or extract some of its logic.

On another note, it looks like since 1.9's addition of RUBY_ENGINE it's easier than it used to be to do this.

[michaelherold]

Hmm ... It seems that the behavior is not totally fixed in 2.2.2. @jrochkind you left the longer specs (outside the loop) below the looped specs. Run like this, they work. However, removing the duplication causes the "unknown key with a suffix" spec to fail on 2.2.2. 😖

To make this more clear, this runs:

    it 'responds to an unknown key with a suffix' do
      %w(= ? ! _).each do |suffix|
        expect(subject).to be_respond_to(:"xyz#{suffix}")
        expect(subject.method(:"xyz#{suffix}")).to_not be_nil
      end

      # for ruby 2.2 - https://github.com/intridea/hashie/pull/285
      expect(subject.method(:"xyz#{'='}")).to_not be_nil
      expect(subject.method(:"xyz#{'?'}")).to_not be_nil
      expect(subject.method(:"xyz#{'!'}")).to_not be_nil
      expect(subject.method(:"xyz#{'_'}")).to_not be_nil
    end

And this fails:

    it 'responds to an unknown key with a suffix' do
      %w(= ? ! _).each do |suffix|
        expect(subject).to be_respond_to(:"xyz#{suffix}")
        expect(subject.method(:"xyz#{suffix}")).to_not be_nil
      end

Also, the ones with the keys that do exist do not fail. This is ... baffling.

Anyway, what I'm going to do is break the two expects into two separate specs and mark the #method one as pending on all versions of MRI 2.2. That way we can run 2.2 on the CI and we'll know if a future version of 2.2 actually fixes the issue since the build will be marked as failed if the spec passes on a 2.2.x version.

I also noticed we're testing an old version of Rubinius, so that's something to check next ...

[jrochkind]

doh! sorry. thanks!