Closed palkan closed 10 years ago
Looks good, I couldn't work out how to test the fix #LearningFromEachOther. I'll accept when I get home unless someone beats me to it.
@palkan Isn't this https://github.com/palkan/date_time_attribute/blob/fix-active-record-bug/spec/active_record_spec.rb#L8 and this https://github.com/palkan/date_time_attribute/blob/fix-active-record-bug/spec/active_record_spec.rb#L37 perform the same thing you contribute?
Not sure if this test case covers https://github.com/einzige/date_time_attribute/commit/8561b7fc11374a9d396318921618894eef3f8d76
It was originally covered here https://github.com/einzige/date_time_attribute/commit/923f7c2418f4e9f606197151014031bac0a3b658
@palkan thanks for the contribution (and giving me the chance to learn by being unstuck) but given the above comments (and as it's einzige's project) I think that at least the following need to be resolved before I would feel comfortable accepting it on einzige's behalf.
@einzige "Isn't this https://github.com/palkan/date_time_attribute/blob/fix-active-record-bug/spec/active_record_spec.rb#L8 and this https://github.com/palkan/date_time_attribute/blob/fix-active-record-bug/spec/active_record_spec.rb#L37 perform the same thing you contribute?"
No those test a model which has been created and saved to the database not one which has been read from the database. In the bug the values of field, field_date, field_time were nil after the record was read from the database.
Also it's a test for https://github.com/einzige/date_time_attribute/commit/8561b7fc11374a9d396318921618894eef3f8d76
create!
should reload record from db.. since it needs new data after INSERT is performed.
Anyway, I will check if specs were failing with new gemfile and no changes in gem code.
btw, @robertgauld did this https://github.com/einzige/date_time_attribute/commit/8561b7fc11374a9d396318921618894eef3f8d76 change specs results? In this PR we implemented specs without changing the code and they are still passing.
@einzige the following approach doesn't do the trick
subject(:target) { Model.create!(created_at: '2014...').reload }
because reload
returns self
with "in-place modifications" (which are absent in the case). So we have to make some sort of find
to really read the record from db.
No, https://github.com/einzige/date_time_attribute/commit/8561b7fc11374a9d396318921618894eef3f8d76 did not change spec results.
Also I can confirm that when running the buggy version
Model.create!(created_at: '2014...').reload.created_at # doesn't return nil
Model.create!(created_at: '2014...').reload.created_at_date # doesn't return nil
Model.create!(created_at: '2014...').reload.created_at_time # doesn't return nil
Model.first.created_at # returns nil
Model.first.created_at_date # returns nil
Model.first.created_at_time # returns nil
@palkan @robertgauld let's make specs fail first to make sure they are really working properly.
ok, guys, I made them fail: https://travis-ci.org/einzige/date_time_attribute/builds/20195582 Merging now, thanks a lot.
Going forward, let's make sure specs are failing before we change the code.
Hi! Just added a test case for handling newly closed bug (8561b7fc11374a9d396318921618894eef3f8d76).