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
Permit running jobs in system tests #36283
Conversation
Inherit from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest. Active Job automatically mixes its test helper into the latter, forcibly setting the test queue adapter before Capybara starts its app server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sorry I didn't answer you before. I was going to say I think the url helpers were required but you included those. As long as basecamp's system tests pass with these changes I think this is fine.
@@ -52,5 +52,11 @@ class Railtie < Rails::Railtie # :nodoc: | |||
|
|||
ActionDispatch.test_app = app | |||
end | |||
|
|||
initializer "action_dispatch.system_tests" do |app| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋Hello!
By including the app.routes.url_helpers
inside the test itself, if you have a route that starts with test_
, minitest will try to call it as it thinks it's a test https://github.com/seattlerb/minitest/blob/ab39d35fb4e84eb866ed9c4ecb707cbf3889de42/lib/minitest/test.rb#L66
It can be surprising to user and can break if the route expect parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. We should include this routes in a object and use method_missing
to delegate to them like we do in the integration tests. @Edouard-chin can you open a PR?
- rails#36283 made a change to make SystemTest inherits from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest. With this change, the route helpers are now directly included inside the SystemTest class. This causes an edge case in case you have a routes whos name starts with `test_`, minitest will consider it as a test and will try to run it https://github.com/seattlerb/minitest/blob/ab39d35fb4e84eb866ed9c4ecb707cbf3889de42/lib/minitest/test.rb#L66 This PR uses a proxy and deleted missing method to a dummy class that has all the route helpers.
- rails#36283 made a change to make SystemTest inherits from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest. With this change, the route helpers are now directly included inside the SystemTest class. This causes an edge case in case you have a routes whos name starts with `test_`, minitest will consider it as a test and will try to run it https://github.com/seattlerb/minitest/blob/ab39d35fb4e84eb866ed9c4ecb707cbf3889de42/lib/minitest/test.rb#L66 This PR uses a proxy and deleted missing method to a dummy class that has all the route helpers.
- rails#36283 made a change to make SystemTest inherits from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest. With this change, the route helpers are now directly included inside the SystemTest class. This causes an edge case in case you have a routes whos name starts with `test_`, minitest will consider it as a test and will try to run it https://github.com/seattlerb/minitest/blob/ab39d35fb4e84eb866ed9c4ecb707cbf3889de42/lib/minitest/test.rb#L66 This PR uses a proxy and deleted missing method to a dummy class that has all the route helpers.
Inherit from
ActiveSupport::TestCase
instead ofActionDispatch::IntegrationTest
. Active Job automatically mixes its test helper into the latter, forcibly setting the test queue adapter before Capybara starts its app server.As a bonus, we no longer need to remove the parts of the
ActionDispatch::IntegrationTest
API we don’t want to expose.