Fix NoMethodError when before_each is reset with nil

[heromoon9218]

Summary

Fix NoMethodError: undefined method 'call' for nil when Grape::Endpoint.before_each nil is called to reset the before_each callbacks.

Problem

After #2643 removed try method usage from the codebase, run_before_each was changed from:

before_each.each { |blk| blk.try(:call, endpoint) }

to:

before_each.each { |blk| blk.call(endpoint) }

This causes a NoMethodError when before_each nil is called.

[github-actions]

Danger Report

No issues found.

View run

[ericproulx]

Hi @heromoon9218, thanks for opening the PR 💪 Could you add a spec to cover this case ? I'm still wondering how it's possible other than calling a callback like before without a block. If so, I will add a runtime error so that a block must be provided.

[heromoon9218]

@ericproulx

Thank you for your feedback 🙏 I've added a spec to cover this case.

My use case:
In my project, I use before_each for test setup/teardown like this:

# Setup
Grape::Endpoint.before_each do |endpoint|
  allow(endpoint).to receive(:current_user).and_return(user)
end

# Teardown
Grape::Endpoint.before_each nil

Why existing specs didn't catch this:
The existing specs (spec/grape/endpoint_spec.rb L36-37, L58-59) expect NameError:

described_class.before_each(nil)
expect { get '/' }.to raise_error(NameError)

Since NoMethodError is a subclass of NameError, existing specs pass despite the bug.

[ericproulx]

I think it would make more sense to put it after the expect. What do you think ?

[heromoon9218]

@ericproulx

I think it would make more sense to put it after the expect. What do you think ?

Just to clarify - are you referring to the new spec at L69, or the existing specs at L36-37 and L58-59 ?

If you mean L69, moving before_each nil after the expect would change the test's intent. In my understanding, the test should verify that a request after before_each nil does not raise NoMethodError.

Apologies if I misunderstood your suggestion 🙏

[ericproulx]

I think I understand the issue with the specs. raise_error(NameError) also captures NoMethodError since the latter inherits from NameError. There are already some tests that should have captured that error. I'll open a PR to fix it. Are you ok with that ?

[heromoon9218]

@ericproulx

There are already some tests that should have captured that error. I'll open a PR to fix it. Are you ok with that ?

Yes, absolutely ! Thank you for your support 🙌

[ericproulx]

@heromoon9218 Could you test #2655 ?

[heromoon9218]

@ericproulx

@heromoon9218 Could you test #2655 ?

I tested #2655 by updating my project's Gemfile to:

gem "grape", github: "ruby-grape/grape", ref: "f31f01d49cb15622c4bd970e1021c40e8276af49"

All tests passed on CI. The fix works as expected. Thank you for your help!

Should I close this PR ?

[ericproulx]

Closing in favor of #2655.