Fix typo in rescue_from unit test (Communication --> Communications).

[xevix]

Typo on one of the unit tests which made it pass falsely (fixing the typo it still passes, but now correctly).

[dm1try]

Ok, but there are another typos: L1314, L1319

@xevix, why not use names the same as in a README (ParentError, ChildError) ?

[dm1try]

Sorry for nitpicking, but this test case is unclear for me:

    it 'does not rescue child errors for other rescue_from instances' do
      class CommunicationsError < RuntimeError; end
      class BadCommunicationError < CommunicationError; end

      class ProcessingError < RuntimeError; end
      class BadProcessingError < ProcessingError; end

      subject.rescue_from CommunicationError, rescue_subclasses: true do |e|
        rack_response("rescued from #{e.class.name}", 500)
      end

      subject.rescue_from ProcessingError, rescue_subclasses: false do |e|
        rack_response("rescued from #{e.class.name}", 500)
      end

      subject.get '/caught_child' do
        raise BadCommunicationError
      end
      subject.get '/uncaught_child' do
        raise BadProcessingError
      end

      get '/caught_child'
      last_response.status.should eql 500
      lambda { get '/uncaught_child' }.should raise_error(BadProcessingError)
    end

if you look closely, you can found the same conditions and assertions here:

    it 'rescues error as well as child errors with rescue_children option set' do
      class CommunicationsError < RuntimeError; end
      subject.rescue_from RuntimeError, rescue_subclasses: true do |e|
        rack_response("rescued from #{e.class.name}", 500)
      end
      subject.get '/caught_child' do
        raise CommunicationsError
      end
      subject.get '/caught_parent' do
        raise RuntimeError
      end
      subject.get '/uncaught_parent' do
        raise StandardError
      end

      get '/caught_child'
      last_response.status.should eql 500
      get '/caught_parent'
      last_response.status.should eql 500
      lambda { get '/uncaught_parent' }.should raise_error(StandardError)
    end

   it 'does not rescue child errors if rescue_subclasses is false' do
      class CommunicationsError < RuntimeError; end
      subject.rescue_from RuntimeError, rescue_subclasses: false do |e|
        rack_response("rescued from #{e.class.name}", 500)
      end
      subject.get '/uncaught' do
        raise CommunicationsError
      end
      lambda { get '/uncaught' }.should raise_error(CommunicationsError)
    end

BTW, It will be better to use context and before blocks.

[dblock]

@dm1try has some good points, would love further iteration from either of you

[xevix]

@dm1try Sorry, this is my fault for wanting to reuse names across unit tests for simplicity, then typo'ing. I will change to use completely different names for exception classes across test cases to avoid this. If I do this, I'll end up not using the same names as in the README (except for one of the tests).

Already in other tests in the same area, I found reuse of error class definitions across tests, so I thought it was safe to do this, but it seems definitions survive between it blocks. See for example:
class CustomError < Grape::Exceptions::Base; end

If context/before blocks will avoid this, I'll refactor to use that (including other tests than the one I wrote, related to rescue_from).

The reason for the first test case combining both was to show that if you use two rescue_from clauses, one with and one without rescue_subclasses true, setting one will have no effect on the other (isolation). This may have been overzealous on my part, but as I was coding, this issue happened because the setting I used was global. I think we can safely remove this test now.

[xevix]

@dm1try Before I go in to fix things, I was hoping you could help me with a few questions:

• Within the scope of describe '.rescue_from' not all (but many) test cases will make use of the exceptions. I can split into contexts of those using one specific exception class definition, and those not, but it seems messy. I'd like to use let but I think that's more for object instances, not classes. What do you think?
• If I find other places where there are other exceptions that are not scoped by context/before and being redefined/redeclared, should I fix them? Or should that be a separate pull request?

Thanks for catching these, and sorry for the trouble.

[xevix]

@dm1try It seems that class definitions can't be scoped within a context/before block. No matter what, using the same name but 2 different class definitions will result in an error, which is unfortunate.

describe 'something' do
  context 'first definition' do
    before do
      class ConnectionError < StandardError; end
    end

    it 'uses the first definition' do
      # ...
      puts "Using class #{ConnectionError}"
    end
  end

  context 'second definition' do
    before do
      class ConnectionError < RuntimeError; end
    end

    it "uses the second definition"
      puts "This test will fail when ConnectionError fails to be redefined"
    end
  end
end

The actual error will look like:

TypeError:
       superclass mismatch for class ConnectionError

So if I use the same name in different areas, I have to ensure the definition is also identical or it will die. I like reusing the same names because it's easier to read, so I'll come up with a branch that does that and ask for your thoughts again.

[xevix]

@dm1try Proposed fixes in #548 . Please have a look when you have a moment. Thanks.

[dm1try]

@xevix, thanks for looking into it!

Yes, I proposed use context/before(/other rspec features) just to avoid code duplication in your specs.
I prefer not to refactor a code that is not mine, but #548 looks better for me.

@dblock can you take a look?

[dblock]

I merged #548. Please don't hesitate to refactor code that's not yours, this is a community project.

[xevix]

@dm1try Thanks for catching things and the suggestions, good stuff.

@dblock Thanks for the quick merge, and I'll try to be more careful about consistency in the future.

[dm1try]

@xevix - good job 💛
@dblock - 👌