Skip to content

Homework - #4

Open
emirdaponte wants to merge 3 commits into
lucashour:masterfrom
emirdaponte:master
Open

Homework#4
emirdaponte wants to merge 3 commits into
lucashour:masterfrom
emirdaponte:master

Conversation

@emirdaponte

Copy link
Copy Markdown

Dos detalles:

  • Actualice la versión del ShouldaMatcher a 4.0.1
  • Y para el test de ApiLoginManager, hice dos versiones. Una siguiendo tu estructura, y la otra como la habría encarado yo.

@emirdaponte emirdaponte changed the title RSpec Training (Emir) Homework May 27, 2019

@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. Me gusta el cambio en los tests del servicio y veo muy bien la iniciativa de proponer algo diferente.

Por cuestiones de tiempos y nivelación, intenté que vean ejemplos claros y fáciles de seguir, quizás por eso notaste o vas a notar que las estructuras no son tan complejas, pero es a propósito. Sin embargo, en casos productivos suelo utilizar algunas de las decisiones que tomaste. Se nota que le has metido a RSpec! 😄

Comment thread spec/models/user_spec.rb
context 'when given value is different from password' do
let(:password) { 'password_wrong' }

it { is_expected.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 sabemos que si falla el resultado es false, por lo que creo que sería más específico con be false.

Comment thread spec/models/user_spec.rb
context 'when given value is equal to password' do
let(:password) { 'password_secret' }

it { is_expected.to be_truthy }

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.

Lo mismo acá, sabemos que debería retornar true.

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

En todos los be_falsey, siendo que el mismo test dice it 'returns false', serían más específicos con un be false.

it 'returns EXTERNAL_VALIDATOR error' do
is_expected.to eq described_class::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.

Me gusta el enfoque! 👍

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

Dejé un comentario que olvidé en el review.

Comment thread spec/models/post_spec.rb
# En base al modelo de test propuesto para el modelo User,
# implementar los tests para el modelo Post.
describe 'Factory' do
subject { build(:post) }

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.

subject { build_stubbed(:post) }

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