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

Ignores use_transactional_fixtures setting #56

Closed
jaredbeck opened this issue Dec 15, 2014 · 6 comments
Closed

Ignores use_transactional_fixtures setting #56

jaredbeck opened this issue Dec 15, 2014 · 6 comments

Comments

@jaredbeck
Copy link
Contributor

MTSR 5.1.1 fails to run ActiveRecord::TestFixtures#setup_fixtures, causing the use_transactional_fixtures setting to be ignored.

Without describe, setup_fixtures is called:

class BananaTest < ActiveSupport::TestCase
  # ..
end

With describe, setup_fixtures is not called:

describe Banana do
  # ..
end

I'd like to use describe.

Reproducible in MTSR 5.0.4 and 5.1.1.

@jaredbeck
Copy link
Contributor Author

To reproduce in a new rails project:

  1. Create file app/models/banana.rb

    module Banana; end
  2. Create file test/models/banana_test.rb

    require 'test_helper'
    
    describe Banana do
      it 'calls setup_fixtures' do
        assert true
      end
    end
  3. Add a debugger breakpoint or puts to setup_fixtures in activerecord-4.1.6/lib/active_record/fixtures.rb

  4. Run the test, see that setup_fixtures is not called

Now, interestingly, if we change our module to a class, then setup_fixtures will be called.

class Banana; end

Maybe describe does not support modules?

@metaskills
Copy link
Owner

Morning @jaredbeck, looking at this now!

@metaskills
Copy link
Owner

@jaredbeck, I really dislike the outer describe test style as it relies on matching the class name to a constant in your Rails app. Done via code like this. In this case, Banana would have to be an ActiveRecord model and likely need to reside in app/models/banana.rb. Is this the case or is Banana some class in lib?

Now, interestingly, if we change our module to a class

When you say that, you mean this right?

require 'test_helper'

class BananaTest < ActiveSupport::TestCase
  it 'calls setup_fixtures' do
    assert true
  end
end

If so, this is the style I recommend since it remains within Rails conventions. I high suggest that you maintain that layer and explicitly subclass and use the Minitest DSL within. I would be open to fixing the outer describe too, based on some answer to the above questions too.

@jaredbeck
Copy link
Contributor Author

I really dislike the outer describe test style as it relies on matching the class name to a constant in your Rails app.

Interesting. Rspec 3 is also moving away from this "test-type inference", as described in this line from the rspec 3.0 release notes.

Spec types are no longer inferred by location, they instead need to be explicitly tagged. The old behaviour is enabled by config.infer_spec_type_from_file_location!, which is still supplied in the default generated spec_helper.rb. (Xavier Shay, Myron Marston)
https://www.relishapp.com/rspec/rspec-rails/v/3-1/docs/changelog

It's a tradeoff. Removing this feature makes the test framework simpler, but requires developers to write more configuration in their test suite.

Regarding the current implementation in MTSR 5.1.1,

.. In this case, Banana would have to be an ActiveRecord model ..

In my reproduction above, app/models/banana.rb is as follows:

module Banana; end

It is not an ActiveRecord model, so I guess that explains why it's not working.

I high suggest that you .. explicitly subclass ..

OK. Let's update the readme to discourage people from using top-level describes. I'll make a PR.

@metaskills
Copy link
Owner

It's a tradeoff. Removing this feature makes the test framework simpler, but requires developers to write more configuration in their test suite.

To be fair, these test classes are typically generated for you by Rails. All you have to do is just type your spec DSL within, but yea, I get the point too :)

OK. Let's update the readme to discourage people from using top-level describes. I'll make a PR.

AWESOME! Try to include some better examples and rational too. Maybe even context link to RSpec like you did. Really looking forward to it!

@jaredbeck
Copy link
Contributor Author

Try to include some better examples and rational too. Maybe even context link to RSpec like you did. Really looking forward to it!

I've gotta get back to work, so I just wrote a simple warning. Feel free to change it / expand on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants