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

tasks.nameに必須と桁数のバリデーション処理を入れる #339

Merged
merged 5 commits into from
Jun 3, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/models/task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@

class Task < ApplicationRecord
enum status: { waiting: 1, work_in_progress: 2, completed: 3 }

validates :name, presence: true, length: { maximum: 20 }
end
7 changes: 7 additions & 0 deletions db/migrate/20190531002052_change_column_to_task.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class ChangeColumnToTask < ActiveRecord::Migration[5.2]
def change
change_column :tasks, :name, :string, limit: 20
end
end
4 changes: 2 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20_190_521_053_254) do
ActiveRecord::Schema.define(version: 20_190_531_002_052) do
create_table 'tasks', options: 'ENGINE=InnoDB DEFAULT CHARSET=utf8', force: :cascade do |t|
t.string 'name', null: false
t.string 'name', limit: 20, null: false
t.text 'description'
t.integer 'status', limit: 1, default: 1, null: false, unsigned: true
t.datetime 'created_at', null: false
Expand Down
37 changes: 37 additions & 0 deletions spec/models/task_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe Task, type: :model do
describe '#save' do
let(:task) { described_class.new(name: 'hoge') }
Copy link

Choose a reason for hiding this comment

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

factorybotを使用すると後々のテストが楽になると思いました。
https://qiita.com/Ushinji/items/522ed01c9c14b680222c

Copy link
Author

Choose a reason for hiding this comment

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

factorybot を使用するように修正しました。
8afae2c


it 'creates records in task' do
task.save
Copy link

@hoooori hoooori Jun 3, 2019

Choose a reason for hiding this comment

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

7行目の下に
before { task.save }
を定義してsave処理を共通化すれば、各テストからtask.saveを削除できるのでコンパクト化できそうです。

Copy link
Author

Choose a reason for hiding this comment

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

#339 (comment)
こちらも合わせて修正しました。

コンパクト化できそうです。

ちなみに個人的にspecは何もかもDRYにする事を目指すより
可読性など見やすさに重きを置いても良いのかなと思ってます。(あくまで個人的にです・・)

Copy link

Choose a reason for hiding this comment

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

修正ありがとうございます。

可読性など見やすさに重きを置いても良いのかな

おっしゃる通りだと思います。
過度なリファクタリングは避けた方が良さそうです。

expect(Task.count).to eq(1)
expect(task.errors.count).to eq(0)
end

context 'nameへの入力がない' do
Copy link

Choose a reason for hiding this comment

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

境界値テストとして1文字の場合もテストした方が良さそうです。

Copy link
Author

Choose a reason for hiding this comment

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

context で括ってない正常系のテストを1文字のケースとして変更しました。
8afae2c

let(:task) { described_class.new(name: nil) }

it '必須のエラーメッセージが出ること' do
task.save

expect(Task.count).to eq(0)
expect(task.errors[:name]).to include('を入力してください')
end
end

context 'nameへ21文字以上の入力があると' do
Copy link

Choose a reason for hiding this comment

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

境界値テストとして20文字の場合もテストした方が良さそうです。

Copy link
Author

Choose a reason for hiding this comment

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

#339 (comment)
こちらも上記に合わせてケースを追加しています。

let(:task) { described_class.new(name: '12345678901234567890a') }
Copy link

Choose a reason for hiding this comment

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

nameは実際のデータを入力するよりも
(name: 'a' * 21)
とした方がコンパクト且つ視覚的に分かりやすいですー

Copy link
Author

Choose a reason for hiding this comment

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

ですね、視覚的にもわかりやすいですね!
#339 (comment)
こちらも合わせて修正しています。


it '桁数超過のエラーメッセージが出ること' do
task.save

expect(Task.count).to eq(0)
expect(task.errors[:name]).to include('は20文字以内で入力してください')
end
end
end
end