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 db state leaking across tests #1384

Merged
merged 2 commits into from
Dec 21, 2015
Merged

Fix db state leaking across tests #1384

merged 2 commits into from
Dec 21, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Dec 20, 2015

See e.g. https://travis-ci.org/rails-api/active_model_serializers/builds/97975719

db records created specs weren't being cleared, so that
other specs calling all had unexpected records in them.

TODO: include a mixin to automatically clear records. Maybe

module TransactionalTests
   # Minitest.run_one_method isn't present in minitest 4
   if $minitest_version > 4 # rubocop:disable Style/GlobalVars
     def run_one_method(*)
       super
     ensure
       ActiveRecord::Base.descendants.each do |record_class|
         record_class.delete_all
       end
     end
  end
end
module SomeTest < Minitest::Test
  extend TransactionalTests
end

Perhaps consider replacing all usages of Minitest::Test with ActiveSupport::Test
for better compatibility.

@bf4 bf4 added the S: Bug label Dec 20, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Dec 20, 2015
- Better minitest 4/5 support
- Better DSL
- Already available with no changes
- Consistent interface

PR has one failure until
rails-api#1384 is
merged.
@bf4 bf4 added the V: 0.10.x label Dec 21, 2015
@bf4
Copy link
Member Author

bf4 commented Dec 21, 2015

The lack of passing builds on master is distracting to active PRs. So, I'm going to merge this.

bf4 added a commit that referenced this pull request Dec 21, 2015
Fix db state leaking across tests
@bf4 bf4 merged commit 9909908 into rails-api:master Dec 21, 2015
@bf4 bf4 deleted the fix_ci_failures branch December 21, 2015 23:23
@beauby
Copy link
Contributor

beauby commented Dec 25, 2015

Raising my voice a bit too late about this, but this way of handling stuff does not seem right at all. We should find a better solution.

@bf4
Copy link
Member Author

bf4 commented Dec 25, 2015

💯

@bf4
Copy link
Member Author

bf4 commented Dec 25, 2015

@beauby Yeah, I would have waited longer, but it did fix the immediate issue, and allowed builds to pass, which was blocking good PRs. I'm not how we should handle 'this PR needs a followup'. Maybe a label? Maybe an issue that lists issues? Maybe a new issue?

@beauby
Copy link
Contributor

beauby commented Dec 30, 2015

Not sure either. A label is probably good.

@bf4
Copy link
Member Author

bf4 commented Dec 30, 2015

Between that and forgetting to use milestones.. I could use a page that says 'start looking for priority work here and here's some stuff not to forget about', or something like that... rails-bot does some cool stuff.. also auto-marks as stale

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.

2 participants