controller specs subject naming convention and context setup best practice

[thilonel]

No description provided.

[Chambeur]

Sometimes, I would prefer use build_stubbed(:model_klass, :factory_trait) instead of ModelKlass.new(attr_1: value_1, attr_2: value_2, attr_3: value_3, attr_4: value_4).

The choice of readability over performance is most of the time related to the implementation context. I don't think this is something worth having in a coding style.

[Azdaroth]

Not enough context here, this needs extra explanation that factories might still be executing some persistence-related stuff when using build_stubbed and in that case, when the data integrity and the state of valid attributes are needed (which would be the case for build_stubbed), ModelKlass.new might be a good alternative.

[thilonel]

This says "prefer over" not "always use".
@Azdaroth is this the place for an explanation? This is a guide to follow, not a book to learn.

Let me extend this explanation for this PR though.

From AR we get ModelKlass.new. This might have initialization callbacks, or other stuff happening that we don't want. Then we can use double(ModelKlass).

When we also need some values set that we don't care about - probably to pass validation - then we can use build_stubbed. Note, that association overrides in factory like association_name { create :association_factory } will be run even when calling stubbed.

When you use build, associations are going to be created. Use this when you need them.

And what you should use least often, is create as its slow. The basic factory of a model must contain just enough data to pass validation and not more.

Being aware of what you create in specs will make you aware of what data you're using in the code. Overusing factories can slow down our spec suite and our code as well.

If you're starting from ModelKlass.new then you're aware of all the data that your object is using. When it becomes hard to setup context, it can be a sign that your object's responsibility is too big, or its interface is clumsy.

[Chambeur]

the subject, called the same as the described method is still a no go for me, especially for readability.
Sometimes you're testing the method process and sometimes you're testing the returned object.
Like

expect { merge }. to raise Something

and

expect(merged_object).to contain something

The naming convention is quite dependent of where it's implemented. I don't think it's a good idea to make the spec looks like it was generated by a bot.

[StoneFrog]

Also in some cases calling it after method reduces readability, especially that method alone doesn't sound good. So I'd agree with Marc, use what sounds best in specific case rather than enforce method name always.
for me expect { call }.to change { Message.count } sounds worse than expect { add_message }.to change { Message.count }

[Azdaroth]

Also, there might be some collision in methods' names in global namespace.

[thilonel]

This is a rule of thumb as it says 2 lines above, start from this and change upon need.
I also change this for controller specs like get_index patch_update post_create etc, where I'm not in control of the API.

For call:

If you can't follow it, stop for a second and think about your object's API, maybe the problem is there.

Why don't you call the method add_message then? Or add? Something more descriptive.

For merge:
You're defining the return value of a method here, not writing english.
expect(merge).to eq ["something"] is fine.

[StoneFrog]

name of the class is descriptive enough in such case and call will allow to use proc as well.
CreateMessage.call(params) is perfectly fine

For merge - you do but if it reads like normal language it's much easier to understand.

[thilonel]

So this is subjective, but still a good rule of thumb.

[StoneFrog]

I agree with rule of thumb, but the part is always the subject, called the same as the described method. (especially word always) does not allow for any exceptions 😛
So basically - I'm ok with using that as long as we can use common sense and apply wording that makes most sense in specific case

[thilonel]

Just read 1 line above.

[StoneFrog]

:doge: didn't notice it refers to whole section 😄

[Chambeur]

I though this was reverted as Seb asked:
https://bookingsync.slack.com/archives/C4WSR1HUJ/p1526725505000069?thread_ts=1526473388.000326&cid=C4WSR1HUJ

I'm still not a fan for the same reason:

IMO, let and let! differences and the calling process are irrelevant for the code readability as it’s designed for test suite execution speed. So I think the common rules of thumbs are more appropriate:

• declare your variable just before where you use it
• group the code lines that belongs together

[StoneFrog]

Agreed with Marc and Seb on this (even though this part is already merged and it was just a grammar change 😄 )

[thilonel]

This probably needs more explanation then as it's coming from the most natural flow of writing specs in RSpec.

You can see how the context name always reflects the setup that's done inside the block.
This order was extracted from writing specs like this, and after months of trial by me and @Azdaroth it seems to work for most cases and eases development.

describe "#foo" do
  subject(:foo) { klass_instance.foo }
  let(:klass_instance) { described_class.new(initial_arguments) }
  
  context "when klass_instance is initialized with this" do
    let(:initial_arguments) { { asdf: true } }
  end
  
  context "when klass_instance is initialized with that" do
    let(:initial_arguments) { { asdf: false } }
    
    context "when some objects in DB are present" do
      let!(:some_object_1) { create :some_object, account: account_1 }
      let(:account_1) { create :account }
      let!(:some_object_2) { create :some_object, account: account_2 }
      let(:account_2) { create :account }
    end
  end
end

[StoneFrog]

most natural flow of writing specs in RSpec. that's your opinion 😛
With described_class I actually more agree with Seb - that it's meaningless to me as well.

let!(:some_object_1) { create :some_object, account: account_1 }
      let(:account_1) { create :account }
      let!(:some_object_2) { create :some_object, account: account_2 }
      let(:account_2) { create :account }

for this part if account_1 would be above some_object_1 it would make no difference for readability, even make it clearer maybe.
With the rest I agree

[thilonel]

I don't care about described class, use it or don't. For me it's easier when renaming, which I do while working on something quite often.

About the order of account and some object, the order is based on the distance of one object to another. some_object is closer to the described class than account is, so usually you just write create :some_object, but in case you need to specify something on the some_object, you'll need another context. Like:

describe "#foo" do
  subject(:foo) { klass_instance.foo }
  let(:klass_instance) { described_class.new(initial_arguments) }

  context "when klass_instance is initialized with this" do
    let(:initial_arguments) { { asdf: true } }
  end

  context "when klass_instance is initialized with that" do
    let(:initial_arguments) { { asdf: false } }

    context "when some objects in DB are present" do
      let!(:some_object_1) { create :some_object, bar: bar_1 }
      let!(:some_object_2) { create :some_object, bar: bar_2 }

      context "when all of their bar is nil" do
        let(:bar_1) { nil }
        let(:bar_2) { nil }
        
        it { is_expected.to eq "..." }
      end

      context "when not all of their bar is nil" do
        let(:bar_1) { nil }
        let(:bar_1) { true }

        it { is_expected.to eq "..." }
      end
    end
  end
end

[StoneFrog]

in case you presented I agree that's the reasonable solution, but it is because the variables you defined later were use to set up specs.
In first example let(:account_1) { create :account } was not influencing the test itself, it looked more like it was just needed for proper general setup. In which case it doesn't matter. Since it's used for setup only it feels more natural to me that flow represents order in which objects are created. therefore I don't think this should be enforced like it's now.

[thilonel]

Yeah, it didn't make sense in the previous example to explicitly create account. Everything is in order of dependency here, your focus is on foo method and you setup whatever is needed for it, but not more. First what you need directly: klass_instance, then what that needs, then what that needs... I've found a few cases only where this order was hard to apply or impossible, I questioned the API of those objects but was not in my power to change it. And again, that's why it's a rule of thumb and you change it when you must. ;)

[Chambeur]

The controller specs are often used as integration tests.
The controller action is not always the subject of the test, that's why it should be a let instead of subject.

Also, I often use the name action like let(:action) { get :index } as it shorten the lines (max column being 100).

[StoneFrog]

for this using get_index, shortening lines shouldn't matter - when specs are quite long just by seeing action it's not clear which action is tested. So here I'd agree with proposed changes.

For cases you mentioned - you can use request specs

[Chambeur]

@StoneFrog Sorry, I'm not sure I understand what you mean by For cases you mentioned - you can use request specs

[StoneFrog]

The controller specs are often used as integration tests. - for that you can use request specs

[Azdaroth]

Not enough context here, this needs extra explanation that factories might still be executing some persistence-related stuff when using build_stubbed and in that case, when the data integrity and the state of valid attributes are needed (which would be the case for build_stubbed), ModelKlass.new might be a good alternative.

[Azdaroth]

Also, there might be some collision in methods' names in global namespace.