Use teardown helper method to ensure that SQLCounter.clear_log is called... #14348

Closed
wants to merge 1 commit into from

2 participants

@senny senny and 1 other commented on an outdated diff Mar 11, 2014
...ecord/test/cases/adapters/mysql/active_schema_test.rb
@@ -1,4 +1,4 @@
-require "cases/helper"
+require 'cases/helper'
@senny
Ruby on Rails member
senny added a note Mar 11, 2014

This is considered a cosmetic change. Our contribution guide states:

Changes that are cosmetic in nature and do not add anything substantial to the stability, functionality, or testability of Rails will generally not be accepted.

This is to preserve git history and make backporting easier. Can you undo that change?

Opps missed that :P Are removing blank lines fine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny senny commented on an outdated diff Mar 11, 2014
...rd/test/cases/adapters/mysql/case_sensitivity_test.rb
@@ -1,5 +1,4 @@
-require "cases/helper"
-require 'models/person'
+require 'cases/helper'
@senny
Ruby on Rails member
senny added a note Mar 11, 2014

removing the superfluous require is fine but the quote change is the same as https://github.com/rails/rails/pull/14348/files#r10464016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member

@tgxworld can you update the PR to contain only the actual change and not all the quote changes?

@tgxworld

@senny I've only managed to go through all the tests within /activerecord/connection_adapters. There are still a whole bunch of tests which are not clearing the log. Is there a guideline if they fix should only be submitted if all the 'bugs' are fixed?

@senny
Ruby on Rails member

@tgxworld blank lines are cosmetic as well. We usually only change cosmetic stuff that appears in the diff anyway (because the underlying code changed).

@senny senny commented on the diff Mar 11, 2014
...ord/test/cases/adapters/postgresql/connection_test.rb
@@ -12,9 +12,8 @@ def setup
@connection = ActiveRecord::Base.connection
end
- def teardown
+ teardown do
@senny
Ruby on Rails member
senny added a note Mar 11, 2014

no need to use teardown do if super is already being called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny senny commented on the diff Mar 11, 2014
...erecord/test/cases/adapters/mysql2/connection_test.rb
@@ -8,9 +8,8 @@ def setup
@connection = ActiveRecord::Base.connection
end
- def teardown
+ teardown do
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member

@tgxworld replacing all def teardown with teardown do is close to a cosmetic change. I'm ok to make sure that super is being called but we should change all Active Record tests in one go.

@tgxworld

@senny Ah ok. If I understand correctly,
This scenario is considered close to a cosmetic change

def teardown
  # something
  super
end

but this shouldn't be right?

def teardown
  # something
end
@senny
Ruby on Rails member

the first example is purely cosmetic. The second is fine to change.

@tgxworld

Ok :) I'll tag you again once I go through all the test. I'm fixing them as I go along making sure that the tests are passing fine with self.test_order.

@tgxworld tgxworld closed this Mar 14, 2014
@tgxworld tgxworld deleted the tgxworld:use_teardown_helper_method_for_active_support_adapters branch Mar 14, 2014
@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 15, 2014
@tgxworld tgxworld Use teardown helper method.
Follow-Up to rails#14348

Ensure that SQLCounter.clear_log is called after each test.

This is a step to prevent side effects when running tests. This will allow us to run them in random order.
3baace6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment