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

Already on GitHub? Sign in to your account

Check for the arity #648

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

ywen commented Dec 3, 2012

I have an active resource model with test.

expect(model).to have(1).error_on(:email)

Before 2.12.0 it works.

But not with 2.12.0. error message:

ArgumentError:
       wrong number of arguments (1 for 0)

failed on:

rspec/rails/extensions/active_record/base.rb
43     self.valid?(options[:context])

ActiveRecord valid? takes one param but not ActiveResource's valid? method.

Contributor

alindeman commented Dec 3, 2012

Thanks for reporting this. It definitely seems like an oversight.

Do you have time to add specs? If not, I can add them before merging but it may be a day or two.

Contributor

ywen commented Dec 3, 2012

I surely can add specs. I did not see any tests for this file so did not add it, sorry

Contributor

alindeman commented Dec 3, 2012

The tests may all be written in terms of cukes, but given this is a bit of an edge case, I feel like a spec is more appropriate.

Thank you!

Contributor

ywen commented Dec 3, 2012

@alindeman I went ahead and added both a rspec and a cuke for this bug fix (which did explode with my old naive fix). Please let me know if they are any good. Thanks!

Contributor

alindeman commented Dec 3, 2012

Great, will review and merge soon. Assigning to myself.

@ghost ghost assigned alindeman Dec 3, 2012

Contributor

ywen commented Dec 3, 2012

https://travis-ci.org/rspec/rspec-rails/jobs/3477802

Not sure what happened here. Can I manually trigger a travis build?

Contributor

alindeman commented Dec 3, 2012

@travis-ci is flaky sometimes. I will run the tests locally before merging.

Contributor

alindeman commented Dec 4, 2012

This is extra annoying since ActiveResource has been extracted in Rails 4. I really feel like it's a bug in ActiveResource, but I'll modify a few things and merge soon.

@alindeman alindeman closed this in 91e9d1e Dec 4, 2012

Contributor

ywen commented Dec 4, 2012

@alindeman should be_valid probably works because the bug (either ActiveResource or rspec's) is not really about that. The problem is in error_on method which should be_valid does not invoke at all, does it?

Contributor

alindeman commented Dec 4, 2012

You're right. I will fix the Changelog entry shortly.

alindeman added a commit that referenced this pull request Jan 7, 2013

`subject.should be_valid` works correctly with ActiveResource models
* ... where valid? does not take an argument
* Closes #648
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment