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

Where to test non-client-facing behaviors from controller actions? #2373

Open
rpbaptist opened this issue Aug 20, 2020 · 16 comments
Open

Where to test non-client-facing behaviors from controller actions? #2373

rpbaptist opened this issue Aug 20, 2020 · 16 comments

Comments

@rpbaptist
Copy link

We were having an internal discussion about this.

Looking at #1839 and considering the documentation has not been changed since it leaves me with questions.

Use request specs to describe the client-facing behavior of the application

Starting background jobs or sending notification mails is not client-facing behaviour. So that should not be covered in request specs. So far so good.

Controller specs can be used to describe the behavior of Rails controllers. As of version 3.5, however, controller specs are discouraged in favor of request specs (which also focus largely on controllers, but capture other critical aspects of application behavior as well).

And here it's stating we should be using request specs for everything previously tested in conrollers. So I am wondering what the recommendation is for non client-facing behaviour. If controller tests are not to be used, and request specs should not test client-facing behavior, then where would you recommend testing them?

This is the kind of tests I currently use for controllers:

expect { post(:create, params: {}) }.to have_enqueued_job(DoSomethingJob).with(model, foo: :bar)
@pirj
Copy link
Member

pirj commented Aug 20, 2020

That's a question that was bothering me as well for a long time.
After all, RSpec Rails is a shim on top of Rails' testing tools. I'm not aware and couldn't find anything reasonable on this topic, including in Rails' own testing guide.

Do you think it makes sense to start a discussion on this topic in the Rails discussion group.
We'll be happy to adjust our recommendations/documentation on this topic to reflect Rails' vision of how this should be done.

Related: rubocop/rspec-style-guide#113 - I've been postponing working on this since there's lack of information/best practices around that.

@pirj
Copy link
Member

pirj commented Aug 20, 2020

It's confusing why we mention ActionController::TestCase::Behavior instead of ActionDispatch::IntegrationTest that Rails's testing guide mentions as the base for controller specs/functional controller tests.

ControllerExampleGroup includes ActionController::TestCase::Behavior, while ActionDispatch::Integration::Runner is included in request and system spec groups only. @JonRowe do you have some recollection on this topic?
I can dig the changes and find the reasoning behind the current state of affairs.

@benoittgt
Copy link
Member

Do you think it makes sense to start a discussion on this topic in the Rails discussion group.

I think this is a good idea :)

@pirj
Copy link
Member

pirj commented Aug 24, 2020

https://discuss.rubyonrails.org/t/need-controller-side-effects-testing-advise/76103

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

Starting background jobs or sending notification mails is not client-facing behaviour. So that should not be covered in request specs. So far so good.

I disagree here, sending notification emails is certainly client facing behaviour, and a background job triggered by a client might not have a visible side effect but is still a side effect of client facing behaviour.

It's confusing why we mention ActionController::TestCase::Behavior instead of ActionDispatch::IntegrationTest that Rails's testing guide mentions as the base for controller specs/functional controller tests.

That link was last updated in 2011, might be time to update it.

ControllerExampleGroup includes ActionController::TestCase::Behavior, while ActionDispatch::Integration::Runner is included in request and system spec groups only. @JonRowe do you have some recollection on this topic?
I can dig the changes and find the reasoning behind the current state of affairs.

I suspect thats related to controller tests being provided by the older Rails / rails-controller-test gem where as our other end-to-end tests use the newer runner.

@rpbaptist
Copy link
Author

rpbaptist commented Aug 26, 2020

I disagree here, sending notification emails is certainly client facing behaviour, and a background job triggered by a client might not have a visible side effect but is still a side effect of client facing behaviour.

There are other background jobs than sending email to the user. It could be an email to the company staff of the application which the client will never notice. It could be a call to an external API of which the result is invisible to the client.

If your stance is to test this in integration tests, that's one thing, but you can't possibly call them client-facing.

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

Client doesn't mean human.

@rpbaptist
Copy link
Author

Client doesn't mean human.

I am aware.

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

I'm open to improving the wording, but client facing here means "behaviour triggered by the outside world".

@rpbaptist
Copy link
Author

My understanding of client-facing is the provided API, whether that's a JSON endpoint or a form in a HTML page. My interpretation of "client-facing behavior" is any behavior which is communicated to that client. Not just triggered by the client.

I could be wrong. I have been searching for a clear definition of the term, but it's not easy to find.

Given that, I still would not check for background jobs being executed in system tests, which I definitly consider to be user-facing.

But I don't really see an advantage in moving tests of side-effects from the controller to a request spec. Or would you advocate checking side-effects in system tests?

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

But I don't really see an advantage in moving tests of side-effects from the controller to a request spec. Or would you advocate checking side-effects in system tests?

Ah you are missing a few pieces of information here, which comes back to my point about improving the documentation.

rspec-rails is a thin wrapper around Rails own test helpers to enable them to be used with RSpec. Controller specs were a wrapper around controller tests which have been deprecated by the Rails team, and thus the wrapper is deprecated by us.

Request specs do largely the same job as controller specs, the main difference being they run through the entirety of the rack stack, where as controller specs did not.

Thus the advantage in moving them is future proofing, having a more realistic behaviour, and being better integration tests (controller specs gave the impression of being a unit test when they were nothing of the sort, they were a partial integration of the rack stack).

At some point controller specs will go away entirely because the mechanisms supporting them will be unsupported.

@fedgut
Copy link

fedgut commented Jan 13, 2021

rspec-rails is a thin wrapper around Rails own test helpers to enable them to be used with RSpec. Controller specs were a wrapper around controller tests which have been deprecated by the Rails team, and thus the wrapper is deprecated by us.

I am currently bringing up to date a large test suite, and I've seen this soft deprecation of the controller specs mentioned elsewhere, but mostly in rails 5.x discussions. In the rails 6 guide, I can't find a reference for this! I'm looking at the guide for v6.10 https://guides.rubyonrails.org/testing.html#functional-tests-for-your-controllers

What I can see is that the Rails team calls them functional tests instead of controller tests. Could this naming convention part of the deprecation? (Old controller tests are no more, functional tests are the new norm?)

If this is the case, then there is no further deprecation coming for RSpec's controller specs, as they are just wrapping the functional tests from Rails.

@JonRowe
Copy link
Member

JonRowe commented Jan 13, 2021

If this is the case, then there is no further deprecation coming for RSpec's controller specs, as they are just wrapping the functional tests from Rails.

This is not the case, controller specs have a different underlying class requiring the rails-controller-testing gem.
Use request specs.

@fedgut
Copy link

fedgut commented Jan 13, 2021

So, the specific deprecated methods from rails 4 to rails 5 where assigns and assert_template?

If I'm not using these methods in my controller specs could I consider them ok?

(asked in the spirit of deepening my understanding, not trying to be repetitive)

@JonRowe
Copy link
Member

JonRowe commented Jan 13, 2021

If I'm not using these methods in my controller specs could I consider them ok?

If you have the rails-controller-testing gem so that it provides the relevant infill.

@fedgut
Copy link

fedgut commented Jan 13, 2021

Ok, I see your point about the need of improving the documentation.

Thank you for your time and expertise! I'll follow your advice and move as many controller specs as I can into request specs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants