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

Better explain the testing style that shoulda-matchers promote #1645

Closed
clemens opened this issue Aug 9, 2024 · 1 comment
Closed

Better explain the testing style that shoulda-matchers promote #1645

clemens opened this issue Aug 9, 2024 · 1 comment

Comments

@clemens
Copy link
Contributor

clemens commented Aug 9, 2024

This is less a feature request than a documentation request.

Also disclaimer: I'm aware that it could be construed as controversial or criticism of one of the most prolific testing gems of the last 1.5 decades. I don't think there's anything wrong with shoulda-matchers and people should be using it if they think it's the best tool for the job. But I do think that especially less experienced engineers deserve some kind of disclosure to prevent 1) later getting bashed by senior engineers for their choice and 2) to prevent cognitive dissonance when reading about unit testing practices elsewhere.


With that out of the way, let me jump into my actual request: I think it would be great if shoulda-matchers would clearly explain the style of testing it promotes and maybe even outline in which cases using the gem might be a good or bad choice.

What I mean is this: Countless blog articles, conference talks, books etc. talk about the importance to test behavior, not implementation in unit tests. Yet shoulda-matchers arguably does the exact opposite in more cases than not.

Examples:

  • Most if not all ActiveRecord association matchers are tested by loading the reflection and making assertions against options.
  • The same holds true for validation matchers (with the notable exception of parts of the uniqueness validation – but arguably this is more done because of how Rails implements the validation rather than as an intentional choice to test behavior over implementation)

And of course the list goes on.

As you're certainly aware, testing implementation instead of behavior mainly hampers the ability to refactor. As a trivial example:

validates :foo, :bar, presence: true
validate :check_required_fields

private

def check_required_fields
  %i[foo bar].each { |field| errors.add(field, :blank) if self[field].blank? }
end

Both approaches do the same thing in terms of behavior, so one could argue that having a unit test that tests these cases shouldn't fail when switching from one approach to the other. But shoulda-matchers would very much fail using the custom (non-idiomatic) way.

If you're looking at it critically, shoulda-matchers could actually be seen as a library that makes use case-based assertions to ensure that the declarations used in your models, controllers etc. conform with idiomatic Rails (as in: if you want to check for the presence of an attribute, Rails says you should use the presence validator, not some custom jazz).

Again: There's nothing wrong with this, especially since it covers these things in a way that tools like RuboCop can't/don't want to. But it's a very specific style of testing where it would be beneficial to be very transparent about the benefits and drawbacks.

I can, of course, provide an MR with suggestions on how these things could be phrased. But I only want to get into that type of work if you generally think it has merit and you don't downright reject my thoughts (which is, of course, your prerogative).

Let me know what you think.

@matsales28
Copy link
Member

Hi @clemens,

Thank you for raising this point. I appreciate your thoughtful feedback and the constructive way you've presented it.

You're absolutely right that testing implementation details can be problematic, especially regarding refactoring. However, the primary goal of shoulda-matchers is to simplify the process of testing common Rails functionality, ensuring that these aspects of your application are correctly configured and functioning as expected.

shoulda-matchers is designed to offer a convenient way to verify that you've set up your models and other Rails components correctly, particularly for common tasks like validations, associations, and more. While it’s true that this can sometimes result in testing implementation details, the intention is to provide a "double-check" that your Rails code adheres to best practices and is free from common errors like misspellings or incorrect option usage.

In my experience, many developers don’t write custom specs for things like associations and other built-in Rails features, often assuming they’ll work as expected. shoulda-matchers helps fill this gap by providing easy-to-use matchers that catch these potential issues.

That said, I do understand the concern that this approach may not align with testing best practices. I agree that it could be helpful to clarify the intended use cases and limitations of shoulda-matchers in the documentation, and I would welcome any contributions you’d like to make in that regard.

Thanks again for your feedback and for considering how we can improve the project together.

clemens added a commit to clemens/shoulda-matchers that referenced this issue Aug 26, 2024
matsales28 added a commit that referenced this issue Sep 6, 2024
* docs: add section on testing style that we promote (#1645)

* Update README.md

Co-authored-by: Matheus Sales <matheus_usales@hotmail.com>

---------

Co-authored-by: Matheus Sales <matheus_usales@hotmail.com>
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

No branches or pull requests

2 participants