Improve consistency with official controller spec style

[i7an]

Some feature specs use a controller spec style that is not the "official" one.

# bad?
describe "GET #index" do
  subject { get :index }

  it "renders the index template" do
    expect(subject).to render_template(:index)
  end
end

# vs.

# good?
describe "GET #index" do
  it "renders the index template" do
    get :index

    expect(response).to render_template(:index)
  end
end

Looking at the code base, subject { get :index } is not used anywhere else. It seems to me that this style has questionable semantics, so aligning the the feature spec content with the common approach for controller specs makes sense to me.

In general, I'd like to hear your opinion on using the get, post, patch call as a subject.

[pirj]

I don’t quite see the problem here. What do you mean by questionable semantics?

[i7an]

As I understand subject, it should be an object that you test. Controller tests usually test response or side-effects of invoking a controller action. Thus putting get, post, patch might be a misuse of subject as you are interested more in side-effects, as if calling subject here is like calling a before block.

Also, I did not find many uses of this pattern, which made me wonder why.

Having said that, I do not have a strong opinion and created this PR basically to find out what is wrong and what is right. However I lean towards eliminating the discrepancy in styles.

UPD. Related? comment from you from a while back.
rspec/rspec-expectations#1106 (comment)

I clearly understand that the composition of those two kinds of expectations when checking side effects of an HTTP requests is non-trivial.

[pirj]

I see.

Rails bends typical RSpec patterns to its needs. Do we test the response, or the interaction here, where the response is the returned result?
Styles of writing specs differ, and we don’t want to impose any specific style as one and only.

I like your suggested change, and I’m inclined to merge it.

However, none of the docs here dictate the style on how to write specs.

[JonRowe]

Our documentation isn't always consistent because its been written at different times and often in different styles, that said we don't typically encourage bare subjects anymore, more typical would be subject(:action) { ... } to make it clear its not the response method.

Also although there is a side effect in that response is changed by get etc, those methods also return the response, which is why it works at all, so I'd actually say subject is fine here.

If you want to name the subject here I'm on board with it but otherwise I think it should be closed to avoid churn.

(An aside, technically controller specs are soft-deprecated in favour of request specs due to Rails decisions).

[pirj]

Thank you!

[JonRowe]

Reverted because as above:

If you want to name the subject here I'm on board with it but otherwise I think it should be closed to avoid churn.