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

Add tests for argument error messages in ActiveRecord methods #42291

Merged

Conversation

martinjaimem
Copy link
Contributor

Summary

  • Added test for the error message of #save!
  • Added test for the error message of #update_all
  • Added test for the error message of #scoping

@zzak
Copy link
Member

zzak commented May 25, 2021

@martinjaimem It seems some tests are failing, can you check it?

Failure:
--
  | PersistenceTest#test_save! [/rails/activerecord/test/cases/persistence_test.rb:306]:
  | --- expected
  | +++ actual
  | @@ -1 +1 @@
  | -"Failed to save the record"
  | +"Validation failed: Content Empty, Title Empty"

https://buildkite.com/rails/rails/builds/77667#63685ee8-b181-44b3-98a1-097069449dbe

@martinjaimem martinjaimem force-pushed the enhancement/add-tests-for-error-messages branch 2 times, most recently from 69be7e2 to c2136a4 Compare May 25, 2021 23:18
@martinjaimem
Copy link
Contributor Author

@martinjaimem It seems some tests are failing, can you check it?

Failure:
--
  | PersistenceTest#test_save! [/rails/activerecord/test/cases/persistence_test.rb:306]:
  | --- expected
  | +++ actual
  | @@ -1 +1 @@
  | -"Failed to save the record"
  | +"Validation failed: Content Empty, Title Empty"

https://buildkite.com/rails/rails/builds/77667#63685ee8-b181-44b3-98a1-097069449dbe

Thanks @zzak , I think I got it right now.

Do you think it's a good idea to improve the error message for the case when the record is already destroyed, and add a custom error message for that?

@martinjaimem martinjaimem force-pushed the enhancement/add-tests-for-error-messages branch from c2136a4 to eec0ff0 Compare May 25, 2021 23:35
@zzak
Copy link
Member

zzak commented May 26, 2021

@martinjaimem LGTM, what is the current error for that case? Could you submit a PR? 🙇

@kamipo kamipo merged commit 56cfefe into rails:main May 26, 2021
@martinjaimem martinjaimem deleted the enhancement/add-tests-for-error-messages branch May 26, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants