Conversation
| @@ -1,16 +1,17 @@ | |||
| source 'https://rubygems.org' | |||
|
|
|||
| ruby '2.6.3' | |||
There was a problem hiding this comment.
когда делаются апдейты разного рода, как здесь, например - их нужно делать не просто разными коммитами, но и разными ПРами, чтобы можно было рассматривать и принимать раздельно
| # frozen_string_literal: true | ||
|
|
||
| class PayrollsController < ApplicationController | ||
| skip_before_action :verify_authenticity_token |
There was a problem hiding this comment.
а с чего бы тут отменять верификацию формы?
это делается только если контроллер принимает внешние запросы
| end | ||
|
|
||
| def create | ||
| CreatePayrollQuery.new(start_date: params[:start_date], end_date: params[:end_date]).invoke |
There was a problem hiding this comment.
в условиях задачи нет ручного ввода даты, поэтому непонятно зачем тут они передаются
| @new_payroll = Payroll.new | ||
| end | ||
| end | ||
| end No newline at end of file |
There was a problem hiding this comment.
отсутствует перенос строки в конце файла, это нарушение code style
| # frozen_string_literal: true | ||
|
|
||
| class CreatePayrollQuery | ||
| DEFAULT_START_DATE = DateTime.new(Time.now.year, Time.now.month, 5) |
There was a problem hiding this comment.
изначально, созданием ДВУХ констант ты ограничиваешь свободу и лишаешь себя возможности гибко настроить систему на одну или три даты
There was a problem hiding this comment.
дублирование кода Time.now.year/Time.now.month
к тому же нет гарантии что эти даты будут в прошлом или будущем
| RSpec.describe CreatePayrollQuery do | ||
| describe '#invoke' do | ||
| it 'should return new pay_roll with first half of month' do | ||
| Payroll.destroy_all |
There was a problem hiding this comment.
это лишнее, в начале тестов БД очищается автоматически
|
|
||
| RSpec.describe CreatePayrollQuery do | ||
| describe '#invoke' do | ||
| it 'should return new pay_roll with first half of month' do |
There was a problem hiding this comment.
давно уже плохой стиль использовать "should" в тестах (потому что масло масляное)
нужно писать в императивном стиле it 'returns new payroll'
если есть какие-то условия, то тест оборачивается в context
context 'with existing payroll' do
it 'creates payroll with closest dates' do
end
end
| last_pay_roll = Payroll.last | ||
|
|
||
| expect(Payroll.count).to eq(2) | ||
| expect(last_pay_roll.starts_at.day).to eq(20) |
There was a problem hiding this comment.
в тесте нет полной проверки даты, так что какой-нибудь 21 год тоже даст зеленый тест, но это будет ошибкой
There was a problem hiding this comment.
разные проверки нужно разносить в разные тесты
context 'with existing payroll' do
before { create payroll }
it 'increases DB entities' do
expect {described_class.new({}).invoke}.to change(Payroll, :count).by(1)
end
it 'creates payroll with correct dates' do
...
end
end
|
|
||
| last_pay_roll = Payroll.last | ||
|
|
||
| expect(Payroll.count).to eq(2) |
There was a problem hiding this comment.
некоторые проверки нет смысла повторять из теста в тест - достаточно одного раза
| expect(last_pay_roll.ends_at.day).to eq(19) | ||
| end | ||
|
|
||
| it 'should return new pay_roll with specific date' do |
There was a problem hiding this comment.
тест должен быть настолько минималистичен, насколько это возможно
все общие места выносятся в before
2 participants
No description provided.