Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing lambda with proc getting argument error because of it. #21244

Merged
merged 1 commit into from
Aug 17, 2015

Conversation

ronakjangir47
Copy link
Contributor

assert_called gives error while calling with block for example

assert_called(object, :deliver!) do
  with_translation 'de', email_subject: '[Anmeldung] Willkommen' do
    ActiveSupport::Deprecation.silence do
      get '/test/send_mail'
    end
    assert_equal "Mail sent - Subject: [Anmeldung] Willkommen", @response.body
  end
end

Before Change

ActionMailerI18nWithControllerTest#test_send_mail:
ArgumentError: wrong number of arguments (1 for 0)
    /Users/ronakjangir/projects/open_source/rails/activesupport/lib/active_support/testing/method_call_assertions.rb:8:in `block in assert_called'

After Change

# Running:

.

Finished in 0.191095s, 5.2330 runs/s, 10.4660 assertions/s.

\cc @kaspth

@kaspth
Copy link
Contributor

kaspth commented Aug 15, 2015

I can't reproduce the error locally. Can you show the full example for the test method? (I assume you're using stub_any_instance somehow, I'd like to see that.)

@ronakjangir47
Copy link
Contributor Author

Here is the full example

def test_send_mail
  stub_any_instance(Mail::SMTP, instance: Mail::SMTP.new({})) do |instance|
    assert_called(instance, :deliver!) do
      with_translation 'de', email_subject: '[Anmeldung] Willkommen' do
        ActiveSupport::Deprecation.silence do
          get '/test/send_mail'
        end
        assert_equal "Mail sent - Subject: [Anmeldung] Willkommen", @response.body
      end
    end
  end
end

@ronakjangir47
Copy link
Contributor Author

@kaspth One more example of this

test "ActionMailer should be told when Mail gets delivered" do
  assert_called(DeliveryMailer, :deliver_mail) do
    DeliveryMailer.welcome.deliver_now
  end
end

Mocha version of this example was

test "ActionMailer should be told when Mail gets delivered" do
  DeliveryMailer.expects(:deliver_mail).once
  DeliveryMailer.welcome.deliver_now
end

I can post more examples if you want to see

@guilleiguaran
Copy link
Member

@ronakjangir47 can we have a test-case for this?

this would be very helpful in the future if someone tries to changed it back to use lambda

@ronakjangir47
Copy link
Contributor Author

@kaspth @guilleiguaran Added test-case. This problem was only with those methods who needs arguments.

@kaspth
Copy link
Contributor

kaspth commented Aug 17, 2015

Didn't understand why that change would fix anything.

I verified it locally with:

irb(main):001:0> stab = -> { puts 'hello' }
=> #<Proc:0x007fa4329fe100@(irb):1 (lambda)>
irb(main):002:0> stab.call(1)
ArgumentError: wrong number of arguments (1 for 0)
    from (irb):1:in `block in irb_binding'
    from (irb):2:in `call'
    from (irb):2
    from /Users/kasperhansen/.rbenv/versions/2.2.2/bin/irb:11:in `<main>'
irb(main):003:0> prok = proc { puts 'hello' }
=> #<Proc:0x007fa4329d7e88@(irb):3>
irb(main):004:0> prok.call 1
hello
=> nil

@kaspth kaspth closed this Aug 17, 2015
@kaspth kaspth reopened this Aug 17, 2015
@kaspth
Copy link
Contributor

kaspth commented Aug 17, 2015

Don't know why Travis didn't put the build here (even after closing/reopening). But: https://travis-ci.org/rails/rails/builds/75992035

kaspth added a commit that referenced this pull request Aug 17, 2015
Replacing lambda with proc getting argument error because of it.
@kaspth kaspth merged commit f7e625b into rails:master Aug 17, 2015
@ronakjangir47 ronakjangir47 deleted the method_call_assertions_fix branch August 18, 2015 00:53
@ronakjangir47
Copy link
Contributor Author

Thanks @kaspth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants