ManageIQ / manageiq

ManageIQ Open-Source Management Platform
https://manageiq.org
Apache License 2.0
1.35k stars 896 forks source link

VimHash confuses RSpec when used where matchers are allowed #11507

Closed cben closed 4 years ago

cben commented 8 years ago

While rewriting a test to expect(hash).to include(k => v, ...) form, I encountered very confusing failures, with non-helpful diff, where I eventually confirmed the expected keys are a strict subset, values for these keys are all ==, the whole structure becomes == if I remove the few extra keys, and yet it was failing! RSpec was even happy with:

expect(data.except(:a_couple, :keys)).to eq(expected)

but not:

expect(data.except(:a_couple, :keys)).to include(expected)

Turns out one of the values inside was a VimHash, and include (unlike eq) is one of many places in RSpec that allow any matcher. Apparently VimHash is taken to be a matcher, but doesn't function correctly as one? Sounds like something that'll bite others...

@miq-bot add-label test, rspec3, technical_debt

I know very little about rspec internals, and nothing about VimHash. ...git praise... @agrare any ideas?

Minimal example

describe 'VimHash is confusing RSpec' do
  let :v1 do
    v1 = VimHash.new
    v1[:foo] = 'bar'
    v1
  end
  let :v2 do # exactly same
    v2 = VimHash.new
    v2[:foo] = 'bar'
    v2
  end
  it 'passes eq' do
    expect(v1).to eq(v2) # passes
  end
  pending 'fails to match, no detail and empty diff :-(' do
    expect(v1).to match(v2)
  end
  pending 'fails to match even itself!  again empty diff :-(' do
    expect(v1).to match(v1)
  end

  context 'Inside bigger data structure:' do
    let :compound do
      {
        :hash => {:foo => 'baz'},
        :vim  => v1,
      }
    end

    it 'passes eq on the whole structure' do
      expect(compound).to eq(:hash => {:foo => 'baz'},
                             :vim  => v1)
    end

    it 'passes include where value is regular item' do
      expect(compound).to include(:hash => {:foo => 'baz'})
    end

    pending 'fails include where value is a VimHash :-(' do
      expect(compound).to include(:vim => v1)
    end

    it 'workaround: explicit eq on the VimHash' do
      expect(compound).to include(:hash => {:foo => 'baz'},
                                  :vim  => an_object_eq_to(v1))
    end
  end
end

output:

.**.*..

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) VimHash is confusing RSpec fails to match even itself!  again empty diff :-(
     # No reason given
     Failure/Error: expect(v1).to match(v1)

       expected {:foo=>"bar"} to match {:foo=>"bar"}
       Diff:

     # ./spec/vimhash_confusing.rb:19:in `block (2 levels) in <top (required)>'

  2) VimHash is confusing RSpec fails to match, no detail and empty diff :-(
     # No reason given
     Failure/Error: expect(v1).to match(v2)

       expected {:foo=>"bar"} to match {:foo=>"bar"}
       Diff:

     # ./spec/vimhash_confusing.rb:16:in `block (2 levels) in <top (required)>'

  3) VimHash is confusing RSpec Inside bigger data structure: fails include where value is a VimHash :-(
     # No reason given
     Failure/Error: expect(compound).to include(:vim => v1)

       expected {:hash => {:foo => "baz"}, :vim => {:foo => "bar"}} to include {:vim => {:foo => "bar"}}
       Diff:
       @@ -1,2 +1,3 @@
       +:hash => {:foo=>"baz"},
        :vim => {:foo=>"bar"},

     # ./spec/vimhash_confusing.rb:40:in `block (3 levels) in <top (required)>'
miq-bot commented 8 years ago

@cben Cannot apply the following label because they are not recognized: technical_debt

cben commented 8 years ago

@miq-bot add-label technical debt

miq-bot commented 7 years ago

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

chessbyte commented 4 years ago

@agrare now that we have abandoned VimHash and VimString outside of the VMware-specific repos, can this issue be closed?

agrare commented 4 years ago

:+1: I removed the last Vim* types from core specs here https://github.com/ManageIQ/manageiq/pull/19833