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

Fix specs about upsert_keys and literal #71

Merged
merged 2 commits into from
Apr 25, 2018

Conversation

manuquentin
Copy link
Contributor

@manuquentin manuquentin commented Apr 25, 2018

A word about the key specs.

In the key specs, an instance was created like : upserted = Vehicle.new(id: v.id, wheels_count: 1).
But this one doesn't pass the validation because the field name is empty.
The upsert returns false because of the error on the validation.
So yes the assertion expect(upserted.id).to eq(vehicule.id) is always true, but nothing happens in the DB: the spec doesn't test anything.

About the literal key.

I miss the thing (in #70) that due to extract_options!, the literal key is removed from upsert_keys and stored in the upsert_options attribute (like the where attribute).
So we don't have to check to class of upsert_key, but just check if the upsert_options constains the literal key.
I trust the specs, but nothing happend in DB.

@olleolleolle
Copy link
Collaborator

(Thanks for the thoughtful repair of the :literal key functionality!)

@@ -44,7 +44,7 @@ module ActiveRecord
end
context 'different ways of setting keys' do
let(:attrs) { {make: 'Ford', name: 'Focus', long_field: SecureRandom.uuid} }
before { Vehicle.create(attrs) }
let!(:vehicule) { Vehicle.create(attrs) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it work with before?

Copy link
Contributor

Choose a reason for hiding this comment

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

The removed line before { Vehicle.create(attrs) } was useless, it should have been written before { @vehicle = Vehicle.create(attrs) }.

Changing let!(:vehicule) { Vehicle.create(attrs) } by let(:vehicule) { Vehicle.create(attrs) } recreate vehicle object on each tests. May be it's better for tests integrity...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current documentation on let and let!:

https://relishapp.com/rspec/rspec-core/v/3-7/docs/helper-methods/let-and-let

Let's keep the French wording in there. And use let!. In this case, we would like to recalculate before each test, so let! it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olleolleolle you're right

@olleolleolle olleolleolle merged commit 3328696 into jesjos:master Apr 25, 2018
@manuquentin manuquentin deleted the fix_literal branch April 25, 2018 10:06
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.

3 participants