Skip to content

controller specs subject naming convention and context setup best practice #7

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.

Sign up for GitHub

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

Open
thilonel wants to merge 2 commits into
base: master
Choose a base branch
from
Open

controller specs subject naming convention and context setup best practice #7

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc85664
controller specs subject naming convention
thilonel Aug 29, 2018
3de1f87
Update README.md
thilonel Aug 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,22 @@ long_line_under_100_chars
# Extra Specs Coding Style

- Add a blank line between `let` and `before`.
- Prefer `ModelKlass.new` over `build_stubbed` over `build` over `create` for optimal performance.
Copy link
Copy Markdown

@Chambeur Chambeur Aug 31, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@Azdaroth Azdaroth Sep 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

- Use the following as a rule of thumb for context setup. If you can't follow it, stop for a second and think about your object's API, maybe the problem is there.

```ruby
# after describe the first is always the subject
# after describe the first is always the subject, called the same as the described method.
Copy link
Copy Markdown

@Chambeur Chambeur Aug 31, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@StoneFrog StoneFrog Aug 31, 2018

Choose a reason for hiding this comment

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

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 }

Copy link
Copy Markdown
Member

@Azdaroth Azdaroth Sep 1, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@StoneFrog StoneFrog Sep 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@StoneFrog StoneFrog Sep 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018

Choose a reason for hiding this comment

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

Just read 1 line above.

Copy link
Copy Markdown

@StoneFrog StoneFrog Sep 4, 2018

Choose a reason for hiding this comment

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

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

# For controller specs use get_index, get_show, patch_update, post_create, delete_destroy, etc.
Copy link
Copy Markdown

@Chambeur Chambeur Aug 31, 2018

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown

@StoneFrog StoneFrog Aug 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@Chambeur Chambeur Aug 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@StoneFrog StoneFrog Aug 31, 2018

Choose a reason for hiding this comment

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

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

describe "#foo"
subject(:foo) { bar.foo }

# then lets follow in the order they will be called when subject is invoked
# then "let"s follow in the order they will be called when subject is invoked
Copy link
Copy Markdown

@Chambeur Chambeur Aug 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@StoneFrog StoneFrog Aug 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@StoneFrog StoneFrog Sep 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@StoneFrog StoneFrog Sep 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@thilonel thilonel Sep 4, 2018

Choose a reason for hiding this comment

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

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. ;)

let(:bar) { Bar.new(baz, qux) }
let(:baz) { build :baz, cruz: cruz }
let(:cruz) { build :cruz }
let(:gux) { build :gux, cruz: cruz }

# let! follows with their lets
# "let!" follows with their "let"s
let!(:corge) { create :corge, grault: grault }
let(:grault) { create :grault }
let!(:garply) { create :garply, grault: grault }
Expand Down