-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Refactor AR's validations_test.rb #10662
Refactor AR's validations_test.rb #10662
Conversation
@@ -14,28 +14,30 @@ class ValidationsTest < ActiveRecord::TestCase | |||
# Other classes we mess with will be dealt with in the specific tests | |||
repair_validations(Topic) | |||
|
|||
def test_error_on_create | |||
def test_valid_question_uses_create_context_when_new_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super-fond of this: took me a bit to parse what valid_question
meant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm i c, what would u prefer than ? :D
@al2o3cr what do u think about this now? :D |
assert r.save, "First save should be successful" | ||
|
||
assert r.valid?, "First validation should be successful" | ||
r.save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just this change is not necessary, it'll run validations twice, you can use r.save in the assert call.
Changes look fine, added minor comments and please squash. Thanks. |
@carlosantoniodasilva thx! udpated and squashed and pushed 😄 |
r = WrongReply.new | ||
r.title = "Wrong Create" | ||
assert !r.save | ||
assert !r.valid? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not assert_not r.valid?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not 😃 thx
fixed and rebased master and force pushed 😃 the failing build is due to some connection problem |
The `:context` switch feature is implemented in ActiveRecord::Validations#valid? method, so we should rename the test names, and execute `valid?` in the test. Change test name in AR's validations_test.rb This test is testing save method's code
…--Validations#valid- Refactor AR's validations_test.rb
Some refactoring, tell me anything if u thought something about this PR :D