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 transient tests failures #970

Merged
merged 1 commit into from
Jun 26, 2015

Conversation

Rodrigora
Copy link
Contributor

These changes fix the issue #961.

I added the timecop gem as suggested by @kurko.

Freezing the time in the two tests should fix the transient failures.


group :test do
gem "timecop"
end
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, any gem required for test to run should be in the gemspec as a development_dependency. your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bf4 Ok. You're right. I'll move to gemspec file. I've found this article after your comment: http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

Another option is remove timecop dependency and use fixed datetimes like: Date.new(2015,6,6). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that would work because it wouldn't affect the updated_at timestamps that Rails adds on save. You'd have to call model.update_column(:updated_at, Time.new(2015,6,6)) after the object was created ( see https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/persistence.rb#L271-L291 ) or whatever the id is in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but in these tests we don't use ActiveRecord inherited classes in tests.
The updated_at method is created "by hand" : https://github.com/rails-api/active_model_serializers/blob/master/test%2Ffixtures%2Fporo.rb#L21

And, the problem here is that it assumes that all code is executed in the same second. That's why sometimes it fails.

Copy link
Member

Choose a reason for hiding this comment

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

Good call

@bf4
Copy link
Member

bf4 commented Jun 25, 2015

Thanks!

@@ -22,5 +22,6 @@ Gem::Specification.new do |spec|

spec.add_development_dependency "rails", ">= 4.0"
spec.add_development_dependency "bundler", "~> 1.6"
spec.add_development_dependency "timecop", ">= 0.7"
Copy link
Member

Choose a reason for hiding this comment

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

@kurko Any thoughts on this PR?

Copy link
Member

Choose a reason for hiding this comment

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

yup, I had suggested it to avoid time related issues.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have concerns about timecop?

@joaomdmoura
Copy link
Member

LGTM, @Rodrigora could you please squash the commits and rebase it? 😁

@Rodrigora Rodrigora force-pushed the fix-test-race-conditions branch from d08e21c to 7412c8d Compare June 26, 2015 12:37
@bf4
Copy link
Member

bf4 commented Jun 26, 2015

@joaomdmoura Looks like @Rodrigora squashed 'em

@Rodrigora
Copy link
Contributor Author

@joaomdmoura, done. 👍

@joaomdmoura
Copy link
Member

Great team work here ppl! Congratz! I'm merging it.

joaomdmoura added a commit that referenced this pull request Jun 26, 2015
@joaomdmoura joaomdmoura merged commit 059409b into rails-api:master Jun 26, 2015
@Rodrigora Rodrigora deleted the fix-test-race-conditions branch July 14, 2015 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants