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

Add advanced test helpers docs to guides #34563

Merged
merged 1 commit into from Dec 4, 2018

Conversation

Projects
None yet
4 participants
@sambostock
Copy link
Contributor

sambostock commented Nov 29, 2018

Summary

This updates the guides to include examples of useful patterns for organizing test helpers, and preventing test_helpers.rb from getting bloated. It adds two sections:

  • Extracting Files, which discusses extracting modules and where they could be put
  • Eagerly Requiring Helpers, which discusses how to easily require these modules for convenience

Motivation

I was unable to find good documentation on how to organize test helpers or custom assertions. Unfortunately, the existing documentation's pattern of adding directly to test_helper.rb can lead to that file becoming bloated. The closest related information I could find was RSpec's documentation on the use of spec/support.

Why not test/support?

In my discussion with @rafaelfranca, I had originally leaned towards test/support. However, in the event that the helpers are complex enough to warrant tests, it is not obvious how this should be handled:

  • Should the tests be in test/support, and explicitly ignored when eagerly requiring the contents of that directory?
  • Should the tests be in another folder, such as test/unit?

@rafaelfranca's suggestion of promoting the helpers to the lib/test directory has an obvious test location of test/lib, making it a good fit for a recommended pattern.


  • Either update #34619 after merging this, or update this after merging #34619

@rails-bot rails-bot bot added the docs label Nov 29, 2018

# test/test_helper.rb
require Rails.root.join('lib', 'test', 'sign_in_helper')
class ActionDispatch::IntegrationTest

This comment has been minimized.

@Edouard-chin

Edouard-chin Nov 29, 2018

Contributor

Use lazy load hook instead

ActiveSupport.on_load(:action_dispatch_integration_test) do
  include SignInHelper
end

This comment has been minimized.

@sambostock

sambostock Nov 29, 2018

Contributor

Interesting. I hadn't come across this before. Is there much benefit here, since by this point test_helper has required the Rails environment already.

I can switch to this if it's the preferred pattern, but I basically just copied the code from earlier in the same guide. Should I switch it in both places?

This comment has been minimized.

@Edouard-chin

Edouard-chin Nov 29, 2018

Contributor

since by this point test_helper has required the Rails environment already

That's fine, loading the Rails environment doesn't mean the ActionDispatch::IntegrationTest class was loaded.
About benefit; referencing constant too prematurely can lead to some bug that are hard to track down, so better to avoid it.

This comment has been minimized.

@gmcgibbon

gmcgibbon Nov 29, 2018

Member

I've never seen this used for test code, but I'm not against it. In your test helper, its fairly safe to assume that ActionDispatch::IntegrationTest is loaded, I think.

This comment has been minimized.

@sambostock

sambostock Dec 4, 2018

Contributor

I could open a follow up PR where I update all the places that re-open IntegrationTest to be consistent (in this file, and in the generated test helper).

This comment has been minimized.

@Edouard-chin

Edouard-chin Dec 4, 2018

Contributor

Sorry! Forgot to answer 😅 .

I think the generated_test_helper.rb isn't up to date, but anyway using a lazy load hook for ActiveSupport::TestCase or even ActionDispatch::IntegrationTest isn't a big deal compare to using a lazy_load_hook for ActiveRecord or ActionController for example, so I'm fine with either :).

@sambostock Feel free to open another PR to make everything consistent

This comment has been minimized.

@gmcgibbon

gmcgibbon Dec 4, 2018

Member

Sounds good to me, we can use consistent syntax for now and discuss changing it in another PR.

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 4, 2018

Member

Yes, it is a big deal. ActionDispatch::IntegrationTest loads ActionController::Base. For test_helpers the current syntax is fine since this is after the environment is loaded and the application is initialized, but for gems it is a problem.

This comment has been minimized.

@sambostock

sambostock Dec 4, 2018

Contributor

Addressed in #34619.

  • If this is merged first, I'll update #34619.
  • If #34619 is merged first, I'll update this.
  • If #34619 is closed without merging, I'll leave this as is.
Show resolved Hide resolved guides/source/testing.md Outdated
Show resolved Hide resolved guides/source/testing.md Outdated
Show resolved Hide resolved guides/source/testing.md Outdated
# test/test_helper.rb
require Rails.root.join('lib', 'test', 'sign_in_helper')
class ActionDispatch::IntegrationTest

This comment has been minimized.

@gmcgibbon

gmcgibbon Nov 29, 2018

Member

I've never seen this used for test code, but I'm not against it. In your test helper, its fairly safe to assume that ActionDispatch::IntegrationTest is loaded, I think.

@sambostock sambostock force-pushed the sambostock:improve-test-helper-guides branch from 8dd57af to 52da90b Dec 4, 2018

@sambostock sambostock referenced this pull request Dec 4, 2018

Closed

Use ActiveSupport.on_load instead of reopening #34619

0 of 2 tasks complete

@sambostock sambostock force-pushed the sambostock:improve-test-helper-guides branch from 52da90b to e8e07d0 Dec 4, 2018

@gmcgibbon
Copy link
Member

gmcgibbon left a comment

👍 Please squash your commits and use [ci skip] to skip Travis

@sambostock sambostock force-pushed the sambostock:improve-test-helper-guides branch from e8e07d0 to d45b74e Dec 4, 2018

@gmcgibbon gmcgibbon merged commit d4ad9b2 into rails:master Dec 4, 2018

1 check passed

codeclimate All good!
Details

@sambostock sambostock deleted the sambostock:improve-test-helper-guides branch Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment