Skip to content

homework 1 - #5

Open
zecee wants to merge 1 commit into
lucashour:masterfrom
zecee:master
Open

homework 1#5
zecee wants to merge 1 commit into
lucashour:masterfrom
zecee:master

Conversation

@zecee

@zecee zecee commented May 27, 2019

Copy link
Copy Markdown

No description provided.

@lucashour lucashour left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Listo el review, no es necesario modificar.

Comment thread spec/models/post_spec.rb
# implementar los tests para el modelo Post.
describe 'Factory' do
it 'has a valid factory' do
expect(build(:post)).to be_valid

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Pese a que el test pasa y está bien, podemos usar un build_stubbed para no persistir al Post en la base de datos (para mejorar la performance). El build es mejor en este sentido que el create y lo veo muy bien, pero aún así persiste aquellas asociaciones que tenga definido el factory. Es sólo un detalle, pero está bueno conocerlo.

expect(build_stubbed(:post)).to be_valid

Comment thread spec/models/user_spec.rb

describe 'Associations' do
# Testear asociaciones (shoulda-matchers).
it { should have_many(:posts) }

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Podemos agregar el test del dependent: :destroy.

describe 'Associations' do
  it { should have_many(:posts).dependent(:destroy) }
end

Comment thread spec/models/user_spec.rb
subject { create(:user) }
context 'when given value is different from password' do
it 'returns false' do
expect(subject.valid_password?(SecureRandom.uuid)).to be_falsey

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Está bien, pero sabiendo que el resultado sería directamente false, podríamos usar be false en lugar de be_falsey. Además, el test dice que retorna false.

subject.call
expect(subject.error).to be(ApiLoginManager::EXTERNAL_VALIDATOR)
end
end

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Creo que modificaría los be_falsey por be false, por el mismo comentario que hice anteriormente.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants