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

Can you support equivalent of rspec before(:all) #61

Closed
gtd opened this issue Oct 20, 2011 · 35 comments
Closed

Can you support equivalent of rspec before(:all) #61

gtd opened this issue Oct 20, 2011 · 35 comments

Comments

@gtd
Copy link

gtd commented Oct 20, 2011

That is a setup that runs once per TestCase. I've done some crazy things to work around this in test/unit over the years, and it's hard to believe that I've never seen it anywhere but rspec. If I have some time I might work on a patch, but I'm buried at the moment. Thoughts?

@zenspider
Copy link
Collaborator

I don't see much utility in it personally. There is probably a good reason you've only seen it in rspec. I've needed it approximately 0 times in 20 years of development and testing. Maybe you should release it as an add-on gem? I'd be happy to refer to it in my README (send me a pull req).

@gtd
Copy link
Author

gtd commented Oct 22, 2011

I'd be curious to understand your approach if you have a slow setup method and you want to test many things about it. Do you just create a single test with a boatload of assertions?

@courtenay
Copy link

Here's specifically where it's used a lot, and the code that (can) fix it. It seems hacky to do this.

http://techno-weenie.net/2010/5/4/escaping-your-test-suite-with-your-life/

Summary: you create a set of fixture objects at the start of a test run for all the tests in that file. You reuse the same objects instead of re-creating them for every single test.

@zenspider
Copy link
Collaborator

From @courtenay's url:

class MyTest < ActiveSupport::TestCase
  fixtures do
    @post = Post.make # <3 Machinist
  end

  test "check something on post" do
    assert_equal 'foo', @post.title
  end

  test "delete post" do
    @post.destroy
  end
end

and why it isn't necessary:

class MyTest < ActiveSupport::TestCase
  @@post = Post.make # <3 Machinist

  test "check something on post" do
    assert_equal 'foo', @@post.title
  end

  test "delete post" do
    @@post.destroy
  end
end

@gtd
Copy link
Author

gtd commented Feb 13, 2012

Thanks for taking the time to write that. I hadn't thought of that approach. You are relying on the test order here to expunge the record and presumably keep the state clean for other tests in the suite. Is that reliable?

@zenspider
Copy link
Collaborator

This code example doesn't rely on test order (and if you're relying on test order for anything, then your tests are broken). None of the examples given have any cleanup and are all sharing the same data across tests.

@gtd
Copy link
Author

gtd commented Feb 13, 2012

That's what I thought too, but if you're defining class variables in the class definition then you're polluting the whole test suite state, no?

@courtenay
Copy link

Well, right, that's a vastly simplified example.

In the linked plugin there's setup and teardown; it wraps the 'before all' in a transaction and the teardown goes into each ivar and does a 'reload' on it. Relevant file: https://github.com/technoweenie/running_man/blob/master/lib/running_man/active_record_block.rb

So that rolls back all the objects you define; so despite them being shared between individual tests, they are only created (the presumably expensive part) once and just reverted each time.

@zenspider
Copy link
Collaborator

@dasil003 -- my example doesn't pollute any more or less the example in @courtenay's url. The point of before(:all) (in this context) is to pollute (ie, cache) so that the test cases may share the data.

@courtenay -- sure, they're both vastly simplified examples (the original and my counter). But is there anything that actually needs/wants before(:all)? I have yet to see a compelling argument for it that isn't solvable using plain ruby. It looks to me that it is more of an argument for an after(:all) since my after_tests is per-process, not per-testsuite.

P.S. Love the name of the project btw. :)

@courtenay
Copy link

It simplifies the setup a lot. For example, something like (from our real app)

  transaction do
    @support    = User.make :state => 'support'
    @site       = @support.site
    @user       = @site.users.make
    @site.public  = false
    @site.save!
  end

Running various methods on the objects and performing manipulations feels pretty dirty to do outside of a method and directly in the class scope.. as does running them at compile-time rather than at run-time. It means that all the setup code has to be abstracted in to a method in the model itself (which isn't necessarily a bad thing). It might work if you could define an initializer, but that doesn't seem to be how test runners work.

Additionally it's a mental crutch; this lets us define two types of setup.
First, the "run once" setup which is where you (expensively) create your objects with factories or whatever, which sets up data for the whole run; and the "run every time" setup where you set/reset things like session/request/..

@zenspider
Copy link
Collaborator

Running various methods on the objects and performing manipulations feels pretty dirty to do outside of a method and directly in the class scope.

There's nothing dirty about code outside of a method... Nothing more dirty than the code that'd go inside of a before(:all). It's just code. And if you want it in a method, put it in a method. That's just code too.

as does running them at compile-time rather than at run-time.

There's no such thing as compile-time evaluation in ruby.

It means that all the setup code has to be abstracted in to a method in the model itself (which isn't necessarily a bad thing).

Nothing mandates that in need to be moved to the model. There's also nothing mandating that this needs to be pushed into setup. Lazy accessors would be ideal with expensive code:

def thingy
  @@thingy ||= do_something_expensive
end

def test_something_else
  assert_equal 42, thingy.something_else
end

It might work if you could define an initializer, but that doesn't seem to be how test runners work.

No, that wouldn't work as MT instantiates a new instance for every test method. Sucks in this regard but it is much safer in the grand scheme of things. It's the only reason (that I could find) why riot out-performs minitest.

Also, I've been experimenting with the idea of test randomization across test classes, which makes things like after(:all) make a lot less sense in the scheme of things... I'm not there yet, but it is under serious consideration.

@zenhob
Copy link

zenhob commented Feb 14, 2012

I should point out: even though there's no real difference between compile time and run time in ruby, there is still order of operations. If you're using machinist then the object being created is instantiated in the database at the time the class is compiled because the call to make is inline in the class code. Rails loads all the test classes at once, then runs each test in turn, wiping or rolling back the database between each test run. The problem is: a database object created at the time the test class is loaded will probably not be available at the time the test is actually run. This is why it's useful to have a callback that will reliably run (even once!) before the tests themselves.

Frankly, this might be an issue that you'll only encounter with legacy test code bases. Even still, it's a valid use case that has no current analog in minitest.

@gtd
Copy link
Author

gtd commented Feb 14, 2012

Ryan, you hit the nail on the head with after(:all) being the key requirement I was going for. I really appreciate your viewpoint on this, so let me just share a concrete example and ask how you would approach it.

I have a test class called SphinxTestCase for testing complex sphinx indexes and queries. The way it works is I set up a few dozen objects, then run the sphinx indexer, then each test makes assertions about the results from a query.

In this case there is massive overhead because of the number of objects created and the indexing overhead. It's literally 100s of milliseconds to setup some of these tests and a couple orders of magnitude less to perform the assertion. Given a lack of after(:all), the only way to do this and still maintain a clean DB and sphinx index for other tests would be to have a single test case with dozens of assertions. I'm not a fussy purist about these things — assert_equal :foo, :bar, "what went wrong" is perfectly acceptable to me semantically, but in this case I really do want to see which assertions are failing all at once rather than fixing one at a time.

I would really appreciate your take on how you would approach this test case, and after that I'll shut up.

@zenspider
Copy link
Collaborator

On Feb 13, 2012, at 19:56 , Zack Hobson wrote:

I should point out: even though there's no real difference between compile time and run time in ruby, there is still order of operations. If you're using machinist then the object being created is instantiated in the database at the time the class is compiled because the call to make is inline in the class code.

I'm going to pick nits here, but only because I think it is important and I think it is causing some confusion on this ticket:

No, it is not at the time it is compiled. It is at the time it is ran. The file is parsed to an AST, compiled to bytecode (on 1.9), and then executed. There is absolutely no concept of compile-time execution. This is important as the file is ran from top to bottom, running everything in the file.

Rails loads all the test classes at once, then runs each test in turn, wiping or rolling back the database between each test run. The problem is: a database object created at the time the test class is loaded will probably not be available at the time the test is actually run.

Let's get concrete... Since we're talking about rails tests, a representative example might be something like:

require "test/helper"

class TestBlah < TestWhatever
  @@expensive_thingy ||= do_expensive_calculation

  def test_something
    assert_equal 42, @@expensive_thingy.something
  end
end

I don't see how this can cause a problem at all. Since the file is always run from top to bottom, test/helper always loads config/environment which in turn does the full rails bootstrap (including database wiring). This is why rails tests are so slow to start up, but also why this hypothetical scenario will never be an issue.

This is why it's useful to have a callback that will reliably run (even once!) before the tests themselves.

I don't know about rspec, but in test/unit and minitest, all tests are run via an at_exit block, which means that anything like @@expensive_thingy would by necessity be ran before the tests run. But, even if minitest didn't use an at_exit block, they'd still be run before the tests, because the method definitions are the only thing ran when the file is executed, not the test code itself...

Frankly, this might be an issue that you'll only encounter with legacy test code bases. Even still, it's a valid use case that has no current analog in minitest.

Shouldn't matter at all. Rails' test helper convention goes way way way back.

@zenspider
Copy link
Collaborator

On Feb 14, 2012, at 05:57 , Gabe da Silveira wrote:

Ryan, you hit the nail on the head with after(:all) being the key requirement I was going for. I really appreciate your viewpoint on this, so let me just share a concrete example and ask how you would approach it.

I have a test class called SphinxTestCase for testing complex sphinx indexes and queries. The way it works is I set up a few dozen objects, then run the sphinx indexer, then each test makes assertions about the results from a query.

In this case there is massive overhead because of the number of objects created and the indexing overhead. It's literally 100s of milliseconds to setup some of these tests and a couple orders of magnitude less to perform the assertion.

But does a rollback take hundreds of milliseconds? I suspect not, in which case it seems fine to have the rollback in a regular teardown/after method. But, as @drbrain just pointed out, you're probably talking about the cost of re-indexing sphinx whenever you do a rollback. Assuming you're doing a rollback at the test level, you need to do a re-index at the test level too, otherwise things will get out of sync and your tests will fail. You'd want to use after(:all) only to undo anything you did that was shared at the class/suite level. In most cases, you need not do anything as just walking away will clean up after itself automatically.

The only time I see a real need for something like after(:all) is when you need to remove a file, close a socket in a particular manner, etc... something stateful that needs cleanup.

Given a lack of after(:all), the only way to do this and still maintain a clean DB and sphinx index for other tests would be to have a single test case with dozens of assertions.

This is your test DB we're talking about. It is made specifically to be messy. It should always set itself up into a known state. How it leaves itself afterwards is usually a non-issue.

But, as (I think) I said before, almost all of this after(:all) discussion is moot as I'm seriously considering adding extra test randomization across test suites. So... I'm not sure how this should play out yet.

@gtd
Copy link
Author

gtd commented Feb 15, 2012

This is your test DB we're talking about. It is made specifically to be messy. It should always set itself up into a known state. How it leaves itself afterwards is usually a non-issue.

I don't disagree with this at all. But you're talking entirely around the issue though. The simple point is that for SphinxTestCases, the setup is two orders of magnitude greater than the query and assertion. That is to say, a test class with 20 cases that I want to assert independently will take 4 seconds if I call setup each time vs 220ms if I call setup once and then reuse it for all the test cases. What I'm saying is that the principle of each test setting up it's preconditions from scratch is worth breaking when we're talking about a 5000% performance penalty.

As long as the test class is loaded and then its tests are run, with no possibility of other test cases running in between those two events, then you are right, I can declare the class-wide initialization in the test class itself. I had never thought to do it this and it is not something I would have naturally tried, the reason being because I assume that if you run a suite, all the test files would be loaded first, then the tests would run. If that is the case, another test that ran first would clobber the class-wise setup. Honestly I've never looked into a test framework close enough to know whether that's true or not, but even if I found out that yes, each class will be dutifully loaded and then run on its own, I still think it's a dangerous implementation detail to rely on.

In fact as you say, you are thinking of taking it a step further for inter-class randomization (if I read you correctly). If that happens then truly the only way to optimize these type of slow setup tests will be to cram all the assertions into a single test.

@scoz
Copy link

scoz commented Feb 19, 2012

Is there some way for the class instance variable method to work with minispec ?

require "minitest_helper"

describe 'An expensive suite' do
  @@expensive ||= -> { puts 'Only once'; 'Expensive!' }.call

  it 'must have an expensive object' do
    @@expensive.wont_be_nil
  end
end

Works, but gives me three warning: class variable access from toplevel

@phiggins
Copy link

@scoz Why not something like:

require "minitest_helper"

describe 'An expensive suite' do
  before do
    @@expensive ||= begin
      puts 'Only once'
      'Expensive!'
    end
  end

  it 'must have an expensive object' do
    @@expensive.wont_be_nil
  end
end

@scoz
Copy link

scoz commented Feb 20, 2012

Either work, both throw warnings(1.9.2 and 1.9.3) on the class variable access as MiniSpec uses anonymous classes.

@brian-goldman
Copy link

I agree with dasil003's most recent comment. I find myself in a very similar situation. Yes, I know I have to be more careful with something like before_all, but it makes testing much faster for a few of my database-related tests.

And dasil003, your assumption is correct: things that are declared at the class level happen before any tests are run. If you were to have separate TestCase subclasses running together with Rake, each with class-level setup operations, these operations would clobber each other before anything runs

I'd like to see this issue reopened because I don't think anyone refuted what dasil003 said. I ended up adding my own method, called setup_once to my subclass of TestCase, and overriding run to make it work. I think it was worth it, and there are situations where it's useful if you understand what you're doing. In my case, I use it to load a very large set of data that the tests treat as read-only.

@zenspider
Copy link
Collaborator

I believe my suggestion to use lazy accessors addressed everything brought up and I'm not sure dasil003 ignored it in his "you're talking entirely around the issue though" comment. class variable access warnings can be easily addressed by not using them. Switch from class variables to class instance variables and the warning goes away:

require "minitest/autorun"

describe 'An expensive suite' do
  def self.expensive
    @expensive ||= begin
                     puts 'Only once'
                     'Expensive!'
                   end
  end

  it 'must have an expensive object' do
    self.class.expensive.wont_be_nil
  end
end

As for the after(:all) part, you can use the after_tests hook if you really need cleanup.

Let me know if I missed something.

@gtd
Copy link
Author

gtd commented Mar 19, 2012

Wait, did you mean I did ignore it?

Anyway, I understand the techniques to do expensive setup once.

The crux of my issue which is really still not addressed is that the sphinx index is a shared resource, I don't want to set up a separate sphinx daemon for each test just so they don't clobber each other. If cross class randomization happens then logically it will be impossible to solve without putting all the assertions in one test.

If that's going forward then it's pointless to continue to debate, but I'll just register my -1 because I think being able to do expensive test setup for groups at a finer grained level than rake tasks is more valuable than the additional reliability of true randomization.

@brian-goldman
Copy link

Ryan, the expensive setup technique is very similar to what I'm currently doing, but there's a difference between users knowing how to do it and having it in the framework as an easy-to-use feature. That's your judgment call, though.

@stid
Copy link

stid commented Jul 25, 2012

If I'm not wrong, the new test/unit support a before all method (def self.startup). This should mean something.

In my personal experience, when you work with external services and you need to test specific and "too complex to simulate" behaviours, a before_all method could save your life.

You should avoid to use it until you really need it. But there are cases where this logic worth the exception.

@zenspider
Copy link
Collaborator

On Jul 25, 2012, at 01:26 , Stefano Furiosi wrote:

If I'm not wrong, the new test/unit support a before all method (def self.startup). This should mean something.

In my personal experience, when you work with external services and you need to test specific and "too complex to simulate" behaviours, a before_all method could save your life.

I've never seen a need for this "life saving" scenario as plain ruby can always solve this problem just fine:

describe Thingy do
  # ... this area reserved for before_all ...

  def lazy_accessor
    # ... this area also reserved for before_all ... sorta
  end

  it "blah blah..."  do
    # ...
  end
end

@stid
Copy link

stid commented Jul 25, 2012

Thanks zenspider.

I missed your previous comment about this. The solution is clear now.

@agraves
Copy link

agraves commented Nov 11, 2012

@zenspider After a lot of thought, I'm on board with the class-level variable approach to handling before-all cases, but the approach you outlined generates warnings on 1.9.3 & minitest spec (as a few other people pointed out).

Is there an accepted variant that doesn't generate warnings?

@zenspider
Copy link
Collaborator

No, the approach I outlined does not generate warnings... Unless I've missed something.

@gramos
Copy link

gramos commented Feb 19, 2013

Hi all, @zenspider I have tried your approach and is raising warnings:

.spec/q_parser_spec.rb:7: warning: class variable access from toplevel

ruby 1.9.3p194
minitest (3.2.0)

I'm using minispec.

@bbozo
Copy link

bbozo commented May 19, 2013

@zenspider, I can't believe no one addressed this, but the case for before(:all) is in transactional DB cleanup strategies and the fact db transactions nest.

When testing against database content it's usually not enough to load up an ivar, you need the rows actually in the database so AR can do it's thing. When you load up data at class load time that data is present in all tests, like:

transaction :all_tests do
  expensive_setup

  transaction :test_1 do
    # the test here can use the data created by expensive setup
  end
end

when rollback happens after test_1, all data loaded up in expensive_setup is still there because only the nested transaction is rolled back.

Global setup methods of the current project I'm working on take a minute to complete and there's 300+ tests, being able to load that data up in test_helper and then use the data in all tests is what makes testing viable.

However, inevitably some of the setup data generation invariably seeps into individual tests for options that depend on the global data being set up in a different way. We're talking about second-long setups on a variety of 20-60-test long suites.

IF it were possible to use a before :all hook then that hook could be used to setup db transactions so that 1-second setup needs to happen only once per test suite and let the rest of the tests run their course and db transactions could get organized as:

transaction :all_tests do
  expensive_global_setup

  transaction :test_suite_1 do
    a_less_expensive_but_still_long_test_suite_specific_setup

    transaction :test_1 do
      # the test here can use the test suite specific setup
    end
  end
end

The lazy accessor approach is no good here because it gets called within the :test_1 db transaction and will get rolled back at the end of each test, prompting another call to it the suite-specific global setup, basically the same thing as a global setup{} block.

If there's a way around this issue, without the before(:all) thingy, I'd be most grateful to hear it, it would shave 5+ minutes off every test run

@courtenay
Copy link

FYI It seems that this may be supported here by building your own plugin: https://github.com/seattlerb/minitest/blob/master/lib/minitest/test.rb#L129

# Runs before every test, before setup. This hook is meant for
      # libraries to extend minitest. It is not meant to be used by
      # test developers.
      #
      # As a simplistic example:
      #
      #   module MyMinitestPlugin
      #     def before_setup
      #       super
      #       # ... stuff to do before setup is run
      #     end

... but I'm still trying to get it to run.

@courtenay
Copy link

Sorry for polluting this thread - but here's a possible solution to the above-metnioned transactional setup "pattern" (hacks ahoy!) in the latest (5.0x series) minitest. It abuses instance variables on the class and then sets them on each instance. https://gist.github.com/courtenay/7343845

@mjacobus
Copy link

@zenspider I don't remember using before(:all) in rspec either. But I would not rule it out as useless. For instance, I am working on a rails generator that has a "run_generator" method that does a lot of things (runs the app generator, some third party generators, bundle install and so on). run_generator takes 4 seconds to run. If minitest had "before(:all)" kind of method it would take "only" 4 seconds to run my current 20 tests instead of 60. I believe this would be a healthy usage of before(:all).

@timruffles
Copy link

@bbozo's use case is why I'm here.

If you're not keen to add it, would you accept a documentation PR to describe how to work around it? For transactional fixtures this is my current workaround:

def setup
  before_all
end

@@before_all_run = false

def before_all
  return if @@before_all_run
  @@before_all_run = true

  # do whatever creates the records - e.g making HTTP requests

  # cache records here to avoid the DB rollbacks in the assertions below
  @@some_record = SomeRecord.find(some_id)
end

def test_something_about_record
    # one assertion
end

 def test_something_else_about_record
    # one assertion
end

# more tests

@science
Copy link

science commented Feb 13, 2015

Per @timruffles and @bbozo comments, I'm a little confused what the dispute is about.

I have an time consuming set of database loads to run. They are the same for every test case in a given suite. I use DatabaseCleaner to rollback changes between each test case. I would like to load that data before the entire suite runs, and then truncate all the data after the entire suite finishes. This will shave quite a bit of time off my full test package.

The reason I want something in the framework is that I use a set of my own classes that my tests suites inherit from depending on what services they need. I have a "DBTest" and and "WebAppTest" which descends from DBTest.

I can engineer this myself, as others have discussed, but having a hook in the test harness would make it simple super simple for me:

class DBTest < MiniTest::Test
  def before_suite 
    super
    Utils.seed_all
  end
  def after_suite
    super
    Utils.truncate_all
  end
end
# then my test suites can just descend from DBTest and get that capability if they want it. 

If I had a choice, I'd greatly prefer the above method hooks to class method calls (before_suite { code }) because it makes it easier to move these things into mix-ins, prevent them from being run in conditional circumstances etc.

After reading through this thread, I'm still not at all clear why this is a bad idea or why Minitest shouldn't support this capability. Obviously it's six feet of rope for people to create heinous test dependencies if they go that route, but there are already lots of others ways to hang your test suite, I think.

@minitest minitest locked and limited conversation to collaborators May 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests