Introduce ApplicationTestCase #28243

gmalette · 2017-03-01T18:55:44Z

I understand this PR isn't complete; I'd like to see if the idea gets traction.

Summary

All Rails test classes extend ActiveSupport::TestCase. This makes it
very had for applications to customize the test inheritance; they simply
have to reopen/freedom-patch ActiveSupport::TestCase.

Having a class implemented by the application would make it much more
straightforward. It's also in line with the way Rails has moved from
ActiveRecord::Base to ApplicationRecord, ActiveJob::Base to
ApplicationJob, etc.

All Rails test classes extend `ActiveSupport::TestCase`. This makes it very had for applications to customize the test inheritance; they simply have to reopen/freedom-patch `ActiveSupport::TestCase`. Having a class implemented by the application would make it much more straightforward. It's also in line with the way Rails has moved from `ActiveRecord::Base` to `ApplicationRecord`, `ActiveJob::Base` to `ApplicationJob`, etc.

rafaelfranca · 2017-03-01T19:01:52Z

I like the idea of generating that in the application but the framework should not use internally. The classes you changed here are actual subclasses of ActiveSupport::TestCase and they should not depend on something that is application specific.

kirs · 2017-03-01T19:06:40Z

activejob/lib/active_job/test_case.rb

@@ -1,7 +1,7 @@
 require "active_support/test_case"

 module ActiveJob
-  class TestCase < ActiveSupport::TestCase
+  class TestCase < ApplicationTestCase


I think the idea of ApplicationTestCase applies to Rails projects, not the Rails test suite itself.

kirs · 2017-03-01T19:07:11Z

railties/lib/rails/generators/test_case.rb

@@ -25,7 +25,7 @@ module Generators
    #     destination File.expand_path("../tmp", File.dirname(__FILE__))
    #     setup :prepare_destination
    #   end
-    class TestCase < ActiveSupport::TestCase
+    class TestCase < ApplicationTestCase


We should also generate ApplicationTestCase for new Rails apps

But ApplicationTestCase make not sense since the application may have view tests, controller tests, integrations tests, model tests. If we add new test case superclasses we would need ApplicationViewTestCase, ApplicationModelTestCase. I think this is a problem to the application solve. If they are doing things to ActiveSupport::TestCase just because it is shared across all they test cases probably they want to add only in the places the thing is needed.

It still means that Rails must provide the structure for that in generators, yes?

gmalette · 2017-03-01T19:21:48Z

@rafaelfranca this is how I see the current hierarchy of Rails test:

As implemented in this PR:

Is this what you're asking?

rafaelfranca · 2017-03-01T19:30:59Z

Yes, exactly that.

rafaelfranca · 2017-03-01T19:32:24Z

Sorry, hit the button really fast. I just think that maybe that is not something that the framework should generate by default. It is too messy in my opinion to have all that inheritance.

@dhh @matthewd options about this one?

matthewd · 2017-03-01T19:45:06Z

I agree this seems very messy.

@gmalette could you show some examples of the monkey-patching this aims to help / alleviate?

gmalette · 2017-03-01T20:00:26Z

@matthewd in our codebase we gradually, throughout the years, added setup on ActiveSupport::TestCase that does a bunch of things like:

deleting a Glob on the FS
loading data from S3 (memoized, but still needs to be done once)
flush memcached
flush redis
inject fixtures into redis
reset elasticsearch

This is fine for controller tests, elastic model tests, etc. However, we have a bunch of objects that don't need access to any form of database. All this setup is not useful to them.

Some people had used tests that extended Minitest::TestCase to get around this setup cost, but they've been removed for reasons @rafaelfranca can explain better me.

Bottom line is, for ~50% (guesstimate) of our tests, we want the costly setup. For the rest, we don't, and we also don't want to use Minitest::TestCase. Ideally these would be replaced by tests inheriting a ActiveSupport::TestCase that's not globally monkey-patched

edit last sentence

gmalette · 2017-03-01T20:13:28Z

To be fair, the simplest way to solve our problem would be to add a layer between unit tests and ActiveSupport::TestCase (all other tests already have another inheritance). We could then monkey-patch that layer instead of ActiveSupport::TestCase itself. I just don't think it's good practice to patch stuff provided by the framework

matthewd · 2017-03-01T20:17:19Z

And how would this PR, in its current form, help you there?

gmalette · 2017-03-01T20:21:14Z

I'd be able to shove all the setup in ApplicationTestCase. We'd be able to have a light class class DBLessTestCase < ActiveSupport::TestCase that doesn't have this setup.

dhh · 2017-03-02T12:18:09Z

We have this exact problem in Basecamp as well. Here's what we currently do in test_helper.rb:

class ActiveSupport::TestCase
  include SignalId::Testing
  include ActiveJob::TestHelper, ActionMailer::TestHelper, ActionCableTestHelper,
          AttachmentTestHelper, BlobPreviewTestHelper, ContentTestHelper, ChatTestHelper, EmailTestHelper, IdentityTestHelper,
          NotificationsTestHelper, ReadingsTestHelper, ReportTestHelper, ScheduledJobTestHelper, TimeTestHelper, UserTestHelper,
          AccountTestHelper

  set_fixture_class playground_prefabs: Playground::Prefab
  fixtures :all

  setup    { FileUtils.mkdir_p BC3::Application.config.x.local_storage.root }
  teardown { FileUtils.rm_rf   BC3::Application.config.x.local_storage.root }

  setup    { FileRepository::Cleversafe::FakeServer.setup! }

  teardown { travel_back }
  teardown { Current.reset }

  teardown { RedisConnectable.clear_all }
  setup    { ApplicationJob.skip_reset_current = true }
  teardown { ActionMailer::Base.deliveries.clear }
  teardown { Rails.cache.clear }
end

class ActionController::TestCase
  include SessionTestHelper, CookieTestHelper

  teardown { Time.zone = "UTC" }

  setup do
    @request.host = accounts('37s').domain
    @request.env['bc3.account.queenbee_id'] = accounts('37s').queenbee_id
  end
end

class ActionDispatch::IntegrationTest
  include ApiTestHelper, SignalId::Testing, SessionIntegrationTestHelper

  setup do
    host! BC3.domain.hostname

    # Nullify the reset so a session can carry on between requests
    Current.singleton_class.send(:alias_method, :real_reset, :reset)
    Current.singleton_class.send(:define_method, :fake_reset) { }
    Current.singleton_class.send(:alias_method, :reset, :fake_reset)

    WebMock.disable_net_connect!(allow_localhost: true)
    WebMock.stub_request(:any, /#{Regexp.escape(Launchpad.url)}/).to_rack(Launchpup.new(Rails.logger))
  end

  teardown do
    Current.singleton_class.send(:alias_method, :reset, :real_reset)
  end

  def page
    Capybara::Node::Simple.new(@response.body)
  end
end

It'd be much better if that configuration happened in ApplicationTestCase and friends. So I'd definitely support this. Any time we're doing mixins and changes like this, it should happen in a Application* class rather than straight on the framework classes.

gmalette · 2017-03-02T13:28:45Z

Here would be my revised plan: change the generators, to introduce ApplicationModelTestCase, ApplicationJobTestCase, ApplicationControllerTestCase, etc. Applications will be free to have a broad-painting application-level mixin (ApplicationTestCase in my diagram) they use in all of these, but not opinionated by Rails.

Ultimately this will introduce a layer between the test cases and Rails.

Good to go?

Also any thought about how to handle the "upgrade"? Will we need to generate those classes in applications? I think yes. I don't think it would be a breaking change, but how resistant will people be to this refactor?

edit: class names

dhh · 2017-03-02T13:44:26Z

As from the thing I posted, I think the biggest value is actually in ApplicationTestCase. At least that's where we have the most specialization. As that's where all the stuff that's shared around fixtures, DB cleanups, etc will live. So I still see ModelTestCase < ApplicationTestCase as a good structure. Yeah, doesn't have to be a breaking change at all. We'd just introduce this for the generators going forward with a flag.

…

On Thu, Mar 2, 2017 at 2:29 PM, Guillaume Malette ***@***.***> wrote: Here would be my revised plan: change the generators, to introduce ModelTestCase, JobTestCase, ControllerTestCase, etc. Applications will be free to have a broad-painting application-level mixin (ApplicationTestCase in my diagram) they use in all of these, but not opinionated by Rails. Ultimately this will introduce a layer between the test cases and Rails. Good to go? Also any thought about how to handle the "upgrade"? Will we need to generate those classes in applications? I think yes. I don't think it would be a breaking change, but how resistant will people be to this refactor? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28243 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtTnsO_kgUrdsEAdDPMvcHrjnb0dDks5rhsQzgaJpZM4MQDwm> .

gmalette · 2017-03-02T14:23:21Z

So I still see ModelTestCase < ApplicationTestCase

😕 I'm not sure how to do that... ? Can we really have both ApplicationControllerTestCase < ApplicationTestCase AND ApplicationJobTestCase < ApplicationTestCase while preserving backward compatibility?

I guess we could move the whole content of all current base test classes to mixins, such that ActionController::TestCase includes ActionController::TestCaseConcern, then generate as

ApplicationControllerTestCase < ApplicationTestCase
  includes ActionController::TestCaseConcern

I'm a bit unconvinced by the use of mixins in application level code, but if you think it's better I don't mind. Unless that's not what you meant and/or have better ideas?

dhh · 2017-03-02T14:27:34Z

Actually, yes, sorry. You're right. The problem is that we have a shared baseclass that we want to specialize but also specialize generic classes below that. Hmm. Don't really like the idea of turning the ActionMailer or whatever into concerns. Maybe what we need is something like ApplicationTestConcerns that's mixed into ActiveSupport::TestCase and then we have ApplicationModelTestCase and such still decent from that. That isn't really much different from reopening ActiveSupport::TestCase directly, though.

…

On Thu, Mar 2, 2017 at 3:23 PM, Guillaume Malette ***@***.***> wrote: So I still see ModelTestCase < ApplicationTestCase 😕 I'm not sure how to do that... ? Can we really have both ApplicationControllerTestCase < ApplicationTestCase AND ApplicationJobTestCase < ApplicationTestCase while preserving backward compatibility? I guess we could move the whole content of all current base test classes to mixins, such that ActionController::TestCase includes ActionController::TestCaseConcern, then generate as ApplicationControllerTestCase < ApplicationTestCase includes ActionController::TestCaseConcern [image: untitled diagram 7] <https://cloud.githubusercontent.com/assets/351915/23510977/a25dffe4-ff29-11e6-96f7-c7e086b99210.png> I'm a bit unconvinced by the use of mixins in application level code, but if you think it's better I don't mind. Unless that's not what you meant and/or have better ideas? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28243 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtY6uGiEvy29-8mBfbGRkQOFdoOmRks5rhtD_gaJpZM4MQDwm> .

gmalette · 2017-03-02T14:40:34Z

Do you dislike specializing all the base classes for the application and let applications decide whether they need shared concerns? Then they can use ApplicationTestConcern and mix it in all the application classes.

class ApplicationMailerTestCase < ActionMailer::TestCase
  include ApplicationTestConcern

dhh · 2017-03-02T14:52:23Z

I think the #1 area where app-concerns cluster currently is in ActiveSupport::TestCase. So whatever solution we arrive at should address that or it's not worth addressing at all.

…

On Thu, Mar 2, 2017 at 3:41 PM, Guillaume Malette ***@***.***> wrote: Do you dislike specializing all the base classes for the application and let applications decide whether they need shared concerns? Then they can use ApplicationTestConcern and mix it in all the application classes. class ApplicationMailerTestCase < ActionMailer::TestCase include ApplicationTestConcern — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28243 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtXRTg5py5_iVn0QYPl7WUZNm3MNXks5rhtURgaJpZM4MQDwm> .

kaspth · 2017-03-06T19:40:15Z

@dhh iirc most of the test cases have a behavior module that they mixin, so they already are concerns.

How do we make sure that the behavior in ApplicationTestCase goes into ActionController::TestCase etc. as well? Since that's currently what happens with plain old class ActiveSupport::TestCase.

dhh · 2017-03-10T12:14:22Z

Hmm, yeah, I think we're going to get a timing issue if we just had a specially named concern that the framework then knew to include. Perhaps there isn't an easy fix here.

kirs · 2017-03-10T15:13:08Z

@dhh I see you're using fixtures :all in Basecamp test_helper. Did you encounter any performance issues related to inserting all fixtures when running the tests locally?

At Shopify, we're using fixtures :all too but now it became a huge pain point because it has to insert 500 fixture sets (that's what we have so far) to run a single test that maybe doesn't even make any database calls. We've been trying to figure out some kind of annotations for every test case whether they rely on persistence layer or not. Right now it may take 11 seconds for a complete run of test/unit/some_pure_ruby_class_test.rb, with 10 sec to insert fixtures and 1 sec to actually run the test.

gmalette · 2018-05-02T20:24:53Z

I'll close this as we couldn't find a good way to do this. I'll be more than happy to pick it up if anyone has suggestions that solves all the concerns.

kirs reviewed Mar 1, 2017

View reviewed changes

maclover7 added needs work railties labels Mar 5, 2017

jackjennings mentioned this pull request Jun 21, 2017

Shared functionality: ApplicationForm? Mixin? Boilerplate? standard-library/microform#1

Open

gmalette closed this May 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ApplicationTestCase #28243

Introduce ApplicationTestCase #28243

gmalette commented Mar 1, 2017

rafaelfranca commented Mar 1, 2017 •

edited

kirs Mar 1, 2017

kirs Mar 1, 2017

rafaelfranca Mar 1, 2017

gmalette Mar 1, 2017

gmalette commented Mar 1, 2017

rafaelfranca commented Mar 1, 2017

rafaelfranca commented Mar 1, 2017

matthewd commented Mar 1, 2017

gmalette commented Mar 1, 2017 •

edited

gmalette commented Mar 1, 2017

matthewd commented Mar 1, 2017

gmalette commented Mar 1, 2017

dhh commented Mar 2, 2017

gmalette commented Mar 2, 2017 •

edited

dhh commented Mar 2, 2017 via email

gmalette commented Mar 2, 2017

dhh commented Mar 2, 2017 via email

gmalette commented Mar 2, 2017

dhh commented Mar 2, 2017 via email

kaspth commented Mar 6, 2017

dhh commented Mar 10, 2017

kirs commented Mar 10, 2017

gmalette commented May 2, 2018

Introduce ApplicationTestCase #28243

Introduce ApplicationTestCase #28243

Conversation

gmalette commented Mar 1, 2017

Summary

rafaelfranca commented Mar 1, 2017 • edited

kirs Mar 1, 2017

Choose a reason for hiding this comment

kirs Mar 1, 2017

Choose a reason for hiding this comment

rafaelfranca Mar 1, 2017

Choose a reason for hiding this comment

gmalette Mar 1, 2017

Choose a reason for hiding this comment

gmalette commented Mar 1, 2017

rafaelfranca commented Mar 1, 2017

rafaelfranca commented Mar 1, 2017

matthewd commented Mar 1, 2017

gmalette commented Mar 1, 2017 • edited

gmalette commented Mar 1, 2017

matthewd commented Mar 1, 2017

gmalette commented Mar 1, 2017

dhh commented Mar 2, 2017

gmalette commented Mar 2, 2017 • edited

dhh commented Mar 2, 2017 via email

gmalette commented Mar 2, 2017

dhh commented Mar 2, 2017 via email

gmalette commented Mar 2, 2017

dhh commented Mar 2, 2017 via email

kaspth commented Mar 6, 2017

dhh commented Mar 10, 2017

kirs commented Mar 10, 2017

gmalette commented May 2, 2018

rafaelfranca commented Mar 1, 2017 •

edited

gmalette commented Mar 1, 2017 •

edited

gmalette commented Mar 2, 2017 •

edited