Add better inspect output for ExampleGroup

[kcdragon]

Pull Request for #1590

The only problem I ran into was that the syntax error in the after hook test produces error output while running the spec (the test still passes).

An error occurred in an after(:context) hook.
NoMethodError: undefined method `expcts' for #<RSpec::ExampleGroups::SomeClass_3 after(:context) hook>

This is definitely not ideal because it could distract from problems with "real" after hooks. The only two options I could up with were: 1) remove the test, or 2) add a way to suppress that error. I'm interested in your thoughts on the best way to handle this.

[myronmarston]

Defining singleton methods blows away the method cache, and besides, we own the class definition of instance, so instead I'd like to see us define inspect directly on the class with the appropriate logic, rather than defining it on each instance.

[kcdragon]

@myronmarston The latest commit removes the call to define_singleton_method and replaces it by storing the inspect output on the instance. I needed to override the instance_variables method to prevent the instance variable storing the inspect output from being set to nil. Let me know if you can think of a better way to handle the inspect output.

[cupakromer]

This constructor change is causing some internal rspec-rails code to fail. I'll have to audit things, but it's possible it's a simple one-liner fix...I'm not sure if I'll have time to get to it before Monday.

[cupakromer]

Ok, looked relatively easy to fix. Opened a PR for it on rspec-rails.

[cupakromer]

I merged into rspec-rails master. Kicked the build to see if it passes now.

[JonRowe]

Typo? This should be expects(1).to eq(1)?

[kcdragon]

It's actually not a typo. I based it on the example the the issue description. See #1590

However, there doesn't need to be an error in the example block in order to produce the inspect output. I'm going to remove the expct calls.

[myronmarston]

Thanks, @kcdragon, this is really close! Besides addressing the last couple comments I left, do you mind squashing it all into one commit?

Also, next time you push updates, can you post a comment mentioning you've done so? Github doesn't notify us when there are new commits and I just happened to notice you had pushed stuff this time.

[myronmarston]

Accessing the instance variable directly like this is a code smell, IMO. I think this spec can be tweaked slightly to retain it's essence without doing that:

      it 'does not clear the inspect output after the example runs' do
        an_example = nil

        line = __LINE__ + 2
        group = ExampleGroup.describe do
          example do
            an_example = self
          end

          before(:context) {}
        end

        group.run

        path = RSpec::Core::Metadata.relative_path(__FILE__)
        expect(an_example.inspect).to end_with("\"example at #{path}:#{line}\">")
      end

Thoughts?

[kcdragon]

I think I'm going to get rid of this spec. It's very similar to:

        it 'includes description and location' do
          an_example = nil

          line = __LINE__ + 2
          group = ExampleGroup.describe 'SomeClass1' do
            example 'an example' do
              an_example = self
            end
          end

          group.run

          path = RSpec::Core::Metadata.relative_path(__FILE__)
          expect(an_example.inspect).to eq("#<RSpec::ExampleGroups::SomeClass1 \"an example\" (#{path}:#{line})>")
        end

[kcdragon]

@myronmarston I added more commits and rebased.

[myronmarston]

So this LGTM (nice work!), and I was going to merge, but then I decided to try it out for the case from #1590. Unfortunately, it doesn't work; when I try the example from #1590, here's what I get:

  1) SomeClass typo in method name
     Failure/Error: expct(3).to eq(3)
     NoMethodError:
       undefined method `expct' for #<RSpec::ExampleGroups::SomeClass:0x007f841506ad10>

Playing around with it jogged my memory and reminded me that I've run across an issue like this before, where a user-defined inspect method is ignored:

https://bugs.ruby-lang.org/issues/8982

As it turns out, NameError#message uses receiver.inspect unless the string returned by receiver.inspect is more than 65 characters; if it's more than 65, it falls back to the default Object#inspect, and ignores the user defined one:

https://github.com/ruby/ruby/blob/v2_1_2/error.c#L1137-L1139

Insane, huh? Not sure if it's possible to achieve the desired behavior :(. Anyone got any ideas?

[kcdragon]

That is really strange. My first thought was to handle the 65 character limit by only returning parts of the inspect output until we reach the limit (example description, then file name, etc.). But then I realized how little 65 characters gives us so I'm not sure if that's even worth it.

[myronmarston]

I thought about possibly truncating it, too, but as you said, that would truncate a lot and it's ridiculous we have to do that.

Anyhow, I'm going to merge this because it's still a big improvement, even if not visible to users when they hit NoMethodError.

[myronmarston]

Thanks, @kcdragon!