Skip to content

Run ActiveModel test suites in random order. #14315

Merged
merged 1 commit into from Mar 9, 2014

6 participants

@zuhao
zuhao commented Mar 7, 2014

This gets the whole ActiveModel test suites working even if
self.i_suck_and_my_tests_are_order_dependent! is disabled
in ActiveSupport::TestCase.

BCrypt::Engine.cost needs to be reset to nil or default,
otherwise changes to cost made in other tests will be carried
over and result in failure when expecting Bcrypt::Engine::DEFAULT.

@senny senny commented on an outdated diff Mar 7, 2014
activemodel/test/cases/secure_password_test.rb
@@ -158,6 +158,7 @@ class SecurePasswordTest < ActiveModel::TestCase
test "Password digest cost defaults to bcrypt default cost when min_cost is false" do
ActiveModel::SecurePassword.min_cost = false
+ BCrypt::Engine.cost = nil
@senny
Ruby on Rails member
senny added a note Mar 7, 2014

what value is cost set to? We need to make sure that every test that modifies the cost actually resets it to it's original value. Otherwise we still have the global state being leaked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arthurnn arthurnn and 3 others commented on an outdated diff Mar 7, 2014
activemodel/test/cases/secure_password_test.rb
@@ -169,6 +169,7 @@ class SecurePasswordTest < ActiveModel::TestCase
@user.password = "secret"
assert_equal BCrypt::Engine.cost, @user.password_digest.cost
+ BCrypt::Engine.cost = nil
@arthurnn
Ruby on Rails member
arthurnn added a note Mar 7, 2014

should this be in a ensure block?

@senny
Ruby on Rails member
senny added a note Mar 7, 2014

@arthurnn it's currently in the wrong place. We need to find the test that leaks state and not the one that depends on it 😄

@zuhao
zuhao added a note Mar 8, 2014

This is actually the test that leaks the global state, isn't it? Bcypt::Engine.cost is explicitly set to 5 in this test at line 168, and if it runs before Password digest cost defaults to bcrypt default cost when min_cost is false, the later will fail because Bcrypt::Engine.cost is no longer Bcrypt::Engine::DEFAULT, but rather 5.

The change here is just an undo of the action, to make sure Bcrypt::Engine.cost is not set to any value prior to other tests (that may assume its having default value).

@htanata
htanata added a note Mar 8, 2014

Just want to confirm that this is the test that leaks the global state. Without this, the assertion at line 163 will fail because the value of BCrypt::Engine.cost is changed by this test.

Setting BCrypt::Engine.cost to nil actually resets it back to the default value. So this looks like a good solution to me.

@senny
Ruby on Rails member
senny added a note Mar 8, 2014

@zuhao you are right of course. I did not get that the Pull Request was actually updated and the diff context did not contain the assignment. Sorry for that 😓

As @arthurnn already mentioned, we need to ensure that this reset is taking place. If the assertion before your reset fails, this will skip the reset and tests running afterwards have a polluted environment. A common pattern is:

  test "Password digest cost honors bcrypt cost attribute when min_cost is false" do
    original_min_cost, ActiveModel::SecurePassword.min_cost = ActiveModel::SecurePassword.min_cost, false
    original_cost, BCrypt::Engine.cost = BCrypt::Engine.cost, 5

    begin
      @user.password = "secret"
      assert_equal BCrypt::Engine.cost, @user.password_digest.cost
    ensure
      ActiveModel::SecurePassword.min_cost = original_min_cost
      BCrypt::Engine.cost = original_cost
    end
  end

Note that this test leaks ActiveModel::SecurePassword.min_cost as well. It might not cause errors down the road but we should clean it up either way. The example above resets both options to it's original state rather than a hardcoded value like nil. This is important because the default might change. If that happens, hardcoded resets will no longer reset to the default but themselves leak the state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny senny self-assigned this Mar 8, 2014
@tgxworld tgxworld referenced this pull request in tgxworld/rails Mar 8, 2014
Closed

Allow activemodel test suites to run in random order #1

@zuhao
zuhao commented Mar 9, 2014

@senny and @arthurnn, thanks a lot for pointing me at the right direction. Indeed, these "state restoration" should either happen in an ensure block, or in teardown method, otherwise the leak is still possible if the assertion fails.

As of now, two potential global state leaks, which cause tests failure when running in random order, have been discovered and fixed for ActiveModel tests.

I've run the whole test suite for ActiveModel for about 1000 times in random order, and no failures occurred.

@senny
Ruby on Rails member
senny commented Mar 9, 2014

@zuhao this is looking good. Can you squash your commits together?

@zuhao zuhao Run ActiveModel test suites in random order.
This gets the whole ActiveModel test suites working even if
`self.i_suck_and_my_tests_are_order_dependent!` is disabled
in `ActiveSupport::TestCase`.

Two places are found that potentially leak global state. This patch
makes sure states are restored so that none of the changes happen in
a single test will be carried over to subsequence tests.
9ffeb36
@zuhao
zuhao commented Mar 9, 2014

@senny just did a squash and force push. This is bad I know. Next time I'll remember to squash into a single commit before sending the pull request.

@senny
Ruby on Rails member
senny commented Mar 9, 2014

@zuhao nothing to worry about. A pull request is "work in progress" this branch belongs to the pull request author and can be manipulated at his will. This includes force pushes. No need to wait with sending the PR. Feel free to force push to existing ones.

@senny senny merged commit 29bd586 into rails:master Mar 9, 2014

1 check was pending

Details default The Travis CI build is in progress
@senny
Ruby on Rails member
senny commented Mar 9, 2014

thank you for your contribution 💛 !

@arthurnn
Ruby on Rails member
@tgxworld tgxworld commented on the diff Mar 10, 2014
activemodel/test/cases/secure_password_test.rb
ActiveModel::SecurePassword.min_cost = true
- @user.password = "secret"
- assert_equal BCrypt::Engine::MIN_COST, @user.password_digest.cost
+ begin
+ @user.password = "secret"
+ assert_equal BCrypt::Engine::MIN_COST, @user.password_digest.cost
+ ensure
+ ActiveModel::SecurePassword.min_cost = original_min_cost
+ end
@tgxworld
tgxworld added a note Mar 10, 2014

@zuhao @senny For this test, ActiveModel::SecurePassword.min_cost does not change at all. Therefore, I think that the changes here are unnecessary and would like to propose that ActiveModel::SecurePassword.min_cost = true be removed since it is being assigned in setup.

Failure in the assertion would still result in ActiveModel::SecurePassword.min_cost being true.

If I am correct, it also seems like we want ActiveModel::SecurePassword.min_cost to stay as true should the assertions fail. In other words, we want the default state of ActiveModel::SecurePassword.min_cost = true. As such, the following call to teardown should be removed too since there seems to be a contradiction with the intention.

  teardown do
    ActiveModel::SecurePassword.min_cost = false
  end

Any thoughts? :)

@zuhao
zuhao added a note Mar 10, 2014

@tgxworld technically, you're right. min_cost does not change in this test, as you pointed out. However, the purpose of this particular test is to ensure "Password digest cost can be set to bcrypt min cost". Therefore, although min_cost is already set to true in setup, it is good to explicitly set it here so that this test is clear. Otherwise, you'll have to refer to setup first, before you can be certain what this is trying to test. That's why I don't recommend removing ActiveModel::SecurePassword.min_cost = true despite it being "redundant".

As for the teardown block, if you take a look at https://github.com/rails/rails/blob/master/activemodel/lib/active_model/secure_password.rb#L8, you'll see that by default min_cost is set to false. We set it to true in setup, and therefore have to restore it back to false to prevent leaks of global state.

@tgxworld
tgxworld added a note Mar 10, 2014

Hi @zuhao , Thanks for clarifying.

Therefore, although min_cost is already set to true in setup, it is good to explicitly set it here so that this test is clear.

I personally think that setup and teardown is essentially part of the tests since it defines the starting state in which the tests are carried out. However, I do agree that this is a matter of personal preference.

As for the teardown block, if you take a look at https://github.com/rails/rails/blob/master/activemodel/lib/active_model/secure_password.rb#L8, you'll see that by default min_cost is set to false. We set it to true in setup, and therefore have to restore it back to false to prevent leaks of global state.

If that is the case, would it be better if it is done this way?

    begin
      @user.password = "secret"
      assert_equal BCrypt::Engine::MIN_COST, @user.password_digest.cost
    ensure
      ActiveModel::SecurePassword.min_cost = false
    end

It is technically more correct to me since when an assertion fails we want the global state returned to it's default state. Also, teardown will still be run even if the assertion fails. Therefore, I just wanted to know if we're being 'too safe' :P

@senny
Ruby on Rails member
senny added a note Mar 10, 2014

setup and teardown do belong to the test-case so it doesn't make much sense to duplicate that logic in an individual test case. However I prefer to have global state modification / cleanup directly in the test methods. If the modification is necessary for multiple test-cases we can easily introduce a helper and don't necessarily need to move it into setup / teardown (See https://github.com/rails/rails/blob/master/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb#L327-L333 for example).

Probably not all tests need min_cost = true to run. Can we try to remove it from setup / teardown and have it explicitly in the tests that need it? Make sure to follow the git history when it was introduced to make sure we are not modifying the intent of the tests.

@tgxworld
tgxworld added a note Mar 10, 2014

Probably not all tests need min_cost = true to run. Can we try to remove it from setup / teardown and have it explicitly in the tests that need it? Make sure to follow the git history when it was introduced to make sure we are not modifying the intent of the tests.

hmm it was introduced to speed up the tests. c2c1ecb

@senny
Ruby on Rails member
senny added a note Mar 10, 2014

I see. In this case I agree with the solution provided by @zuhao . The tests that do rely on an specific value of min_cost should set it inside the test methods. The ones that don't care inherit the setup to speed tests up. We should:

  • store the original value of min_cost in setup and assign it back in teardown. (@original_min_cost = ...). This will get rid of hardcoded flags.
  • use a descriptive comment to document the assignment in setup (that it is for performance reasons only).
@chancancode
Ruby on Rails member
chancancode added a note Mar 10, 2014

I think I overtook git blame ownership when I rewrote the tests earlier this year (😢), so I'll quickly chime in :) The reason for setting min_cost on these tests is to supposedly speed things up when any test case directly or indirectly trigger a password=. A good number of them ended up requiring it (~20 out of 23), so it was extracted to the setup/teardown hooks. This also has another small advantage – when someone add a new test case they can't "forget" to use a helper method etc to accidentally introduce a slow test.

So while I usually agree that inlining these changes is better, in this case it might be justified. Not a huge difference though.

As for the other things being discussed, I have no strong opinions one way or the other.

@chancancode
Ruby on Rails member
chancancode added a note Mar 10, 2014

Oops, my comment was written before I saw the latest replies, ignore me :)

+1 for "store the original value of min_cost"

@tgxworld
tgxworld added a note Mar 11, 2014

This also has another small advantage – when someone add a new test case they can't "forget" to use a helper method etc to accidentally introduce a slow test.

@senny I agree with the above point that @chancancode made. Furthermore, I've dug deeper into MiniTest::Test and realized that we should not have to handle cleanup directly in the test methods. Instead, MiniTest was written with the intent that even if an assertion fails or an error was raised, teardown will still be executed. Therefore, resetting the modification of global states could be handled in teardown with confidence. The benefit of doing so is that the tests will become DRYer and makes the test more readable since the starting and ending states are well defined in setup and teardown respectively.

    # minitest/lib/minitest/test.rb
    def run
      with_info_handler do
        time_it do
          capture_exceptions do
            before_setup; setup; after_setup

            self.send self.name
          end

          %w{ before_teardown teardown after_teardown }.each do |hook|
            capture_exceptions do
              self.send hook
            end
          end
        end
      end

      self # per contract
    end

    def capture_exceptions # :nodoc:
      begin
        yield
      rescue *PASSTHROUGH_EXCEPTIONS
        raise
      rescue Assertion => e
        self.failures << e
      rescue Exception => e
        self.failures << UnexpectedError.new(e)
      end
    end

Let me know your thoughts :)

@senny
Ruby on Rails member
senny added a note Mar 11, 2014

@tgxworld I'm fine with global state modifications inside setup as long as every test depends on it. We should not modify more state than absolutely necessary. That's why I favor helper methods within the test method.

In this particular case it's good to modify min_cost in setup / teardown because it's for performance reasons. However tests that do rely on an explicit value of min_cost should still assign it to be explicit.

@tgxworld
tgxworld added a note Mar 11, 2014

Ah ok :) got it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zuhao zuhao deleted the zuhao:activemodel_tests_in_random_order branch May 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.