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

Adds assert_error and assert_no_error #51774

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DanielaVelasquez
Copy link

@DanielaVelasquez DanielaVelasquez commented May 9, 2024

Motivation / Background

Ensuring the correctness of validations in Rails models is a fundamental aspect of robust testing. However, the current approach to such validations can often lead to cluttered and less readable test code. For instance, manually asserting validation errors requires multiple lines of code, detracting from the clarity of the test suite. This PR addresses this issue by introducing a streamlined solution that enhances readability and conciseness in model testing.

test "name cannot be blank" do
    user = users(:one)
    
   user.validate
   refute_empty user.errors.where(:name, :blank)
end

or

test "name cannot be blank" do
    user = users(:one)
    
   user.validate
   assert user.errors.added? :name, :blank
end

Detail

This PR introduces a new assertion method, assert_error and assert_invalid, designed to simplify the validation testing process. With them developers can now verify the presence of specific validation errors with a single, expressive line of code. This enhancement not only improves the readability of tests but also promotes better understanding and maintenance of the validation logic within Rails models.

test "name cannot be blank" do
    user = users(:one)
    
   assert_not_valid :name, :blank, user
end
test "name cannot be blank" do
    user = users(:one)
    user.validate
    
   assert_error :name, :blank, user
end

In the same way the validation assert_valid and assert_no_error ensures that the model has no errors for a specific field

test "name is not blank" do
    user = users(:one)
    
   assert_no_error :name, :blank, user
end
test "name is not blank" do
    user = users(:one)
    
   assert_valid :name, :blank, user
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

# Assertion that an active model has a specific invalid field
#
# assert_invalid :name, :blank, :user
def assert_invalid(attribute, type, obj, msg = nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this, having only assert_invalid feels like it doesn't match the framework's other assertions. I think the framework should support assert_valid and assert_not_valid. What do you think about changing this PR to support those?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea, thank you for the feedback. I updated it and changed the name of the method to make it more descriptive so it's clear that it references errors in the model.

@DanielaVelasquez DanielaVelasquez changed the title Adds assert_invalid Adds assert_error and assert_no_error May 11, 2024
@rafaelfranca
Copy link
Member

Does this work when the error is added as a string instead of symbol? errors.add(:name, "Some error")

Comment on lines 264 to 324
# Assertion that an active model doesn't have an error on a field
#
# assert_no_error :name, :blank, :user
def assert_no_error(attribute, type, obj, msg = nil)
raise ArgumentError.new("#{obj.inspect} does not respond to #validate") unless obj.respond_to?(:validate)

obj.validate
msg = message(msg) {
data = [attribute, type]
"Expected %s to not be %s" % data
}
assert_empty obj.errors.where(attribute, type), msg
end

# Assertion that an active model has a specific error on a field
#
# assert_error :name, :blank, :user
def assert_error(attribute, type, obj, msg = nil)
raise ArgumentError.new("#{obj.inspect} does not respond to #validate") unless obj.respond_to?(:validate)

obj.validate
msg = message(msg) {
data = [attribute, type]
"Expected %s to be %s" % data
}
refute_empty obj.errors.where(attribute, type), msg
end
Copy link
Contributor

@seanpdoyle seanpdoyle May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To share some prior art in this space, thoughtbot/shoulda-matchers might be of interest. It is a 14 year old project that provides Active Model validation assertions (amongst other things).

Like this proposal, it also invokes #validate on the object directly. Unlike this proposal, it names its matches in a way that communicates that it's invoking #validate (for example, validates_presence_of, validates_uniqueness_of, etc.).

Callers of an assert_no_error assertions might be surprised to learn that it's also invoking #validate, since #validate clears any errors present on the object before re-validating. Depending on the details of the test itself, that kind of reseting might un-do and remove errors added during that test.

As a caller, I might expect assert_no_error to only make assertions about the errors available by the subject under test's #errors method.

If this proposal is deemed viable, what do you think about introducing both assert_valid and assert_not_valid assertions that invoke assert_no_error and assert_error (respectively)? That way, callers can benefit from the convenience of assert_valid or assert_not_valid while also being able to drop down to the finer-grained assert_error or assert_no_error.

I'm imagining something like:

      # Assertion that an active model doesn't have an error on a field
      #
      #   assert_no_error :name, :blank, :user
      def assert_no_error(attribute, type, obj, msg = nil)
        msg = message(msg) {
          data = [attribute, type]
          "Expected %s to not be %s" % data
        }
        assert_empty obj.errors.where(attribute, type), msg
      end
      
      # Assertion that an active model has a specific error on a field
      #
      #   assert_error :name, :blank, :user
      def assert_error(attribute, type, obj, msg = nil)
        msg = message(msg) {
          data = [attribute, type]
          "Expected %s to be %s" % data
        }
        refute_empty obj.errors.where(attribute, type), msg
      end
      
      def assert_not_valid(attribute, type, obj, msg = nil)
        raise ArgumentError.new("#{obj.inspect} does not respond to #validate") unless obj.respond_to?(:validate)

        obj.validate
        assert_error(attribute, type, object, msg)
      end

      def assert_valid(attribute, type, object, msg = nil)
        raise ArgumentError.new("#{obj.inspect} does not respond to #validate") unless obj.respond_to?(:validate)

        obj.validate
        assert_empty obj.errors, msg
      end

It's worth mentioning that assert_valid doesn't exactly mean "there are no errors under this particular key", it means "there are no errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there plans to support Active Model Validation Contexts?

When invoking the version of assert_no_error that does not call #validate, it's up to the test code that precedes the assertion to validate with the proper context.

When invoking the assert_valid and assert_not_valid variations, there isn't a way to forward along any contextual information (like an :on option). This is also true of the currently proposed assert_error and assert_no_error definitions that call #validate directly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callers of an assert_no_error assertions might be surprised to learn that it's also invoking #validate
Agreed, thanks for the feedback. I will go ahead and make those changes.

Regarding

Are there plans to support Active Model Validation Contexts?

This can be another type of assertion. However, the current change aims to focus on a model that went through the process of doing its validations.

PS: This is my first contribution ever to Rails and if it's better to ship a more defined feature then I am onboard with adding more stuff. LMK what's the approach generally taken. I am thinking this in terms of small deliverable.

@DanielaVelasquez DanielaVelasquez force-pushed the assert-invalid branch 2 times, most recently from 192aebd to c7d35c1 Compare May 15, 2024 00:51
@rails-bot rails-bot bot added the railties label May 15, 2024
@DanielaVelasquez DanielaVelasquez force-pushed the assert-invalid branch 4 times, most recently from 0568062 to 3cde006 Compare May 15, 2024 01:12
@DanielaVelasquez
Copy link
Author

Does this work when the error is added as a string instead of symbol? errors.add(:name, "Some error")

It does, since it's using added to make the assertions

@eileencodes
Copy link
Member

I'm fine being veto'd by someone else on Core but I don't like assert_error and assert_no_error because I think it's too close to assert_raises and assert_nothing_raised.

assert_validation_error and assert_no_validation_error is maybe better? Kind of a mouthful though 🤔

@zzak
Copy link
Member

zzak commented May 18, 2024

If the name is generic enough like this, and it really only applies to ActiveModel objects, then it should only be available in something like ActiveModel::TestCase 🤔

@DanielaVelasquez
Copy link
Author

If the name is generic enough like this, and it really only applies to ActiveModel objects, then it should only be available in something like ActiveModel::TestCase 🤔

@zzak, the problem with that approach is that then tests would have to inherit from something like ActiveModel::TestCase and rails is already pretty well-established on using ActiveSupport::TestCase , I don't know if we want to change that, which is why I was leaning towards leaving it in ActiveSupport, what do you think?

@DanielaVelasquez DanielaVelasquez marked this pull request as draft October 2, 2024 22:40
@DanielaVelasquez DanielaVelasquez force-pushed the assert-invalid branch 2 times, most recently from fa2907d to 648e297 Compare October 3, 2024 21:58
@rails-bot rails-bot bot added the actioncable label Oct 3, 2024
@DanielaVelasquez DanielaVelasquez force-pushed the assert-invalid branch 2 times, most recently from fa2907d to 655f8c5 Compare October 3, 2024 22:05
@zzak
Copy link
Member

zzak commented Oct 4, 2024

Yes, I realized that after I made the comment, sorry was just thinking about it.
You're 100% right in that model tests inherit from AS::TestCase (by design).

Would you mind fixing the failing tests / lint errors?
This one in particular looks interesting, you should be able to reproduce it locally with RAILS_STRICT_WARNINGS=1 bin/test test/testing/assertions_test.rb:

/rails/activesupport/test/testing/assertions_test.rb:7: warning: method redefined; discarding old setup
/rails/activesupport/lib/active_support/testing/strict_warnings.rb:38:in `warn': /rails/activesupport/test/testing/assertions_test.rb:7: warning: method redefined; discarding old setup (ActiveSupport::RaiseWarnings::WarningError)
	from /rails/activesupport/test/testing/assertions_test.rb:7:in `<class:AssertionsTest>'
	from /rails/activesupport/test/testing/assertions_test.rb:6:in `<top (required)>'
	from <internal:/usr/local/lib/ruby/site_ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:37:in `require'
	from <internal:/usr/local/lib/ruby/site_ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:37:in `require'
	from /usr/local/bundle/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:21:in `block in <main>'
	from /usr/local/bundle/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in `select'
	from /usr/local/bundle/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in `<main>'

@DanielaVelasquez DanielaVelasquez force-pushed the assert-invalid branch 5 times, most recently from 1e29075 to ee2e91e Compare October 12, 2024 00:22
Introduces two new assertions, `assert_error_on` and `assert_no_error_on`, to simplify checking for specific validation errors on models.

Example usage:
  - assert_error_on user, :name, :blank
  - assert_no_error_on user, :name, :blank

This enhances test readability and makes validation testing more intuitive.
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.

5 participants