Run integration tests through the app's executor#24608
Conversation
|
See also cf075d9 |
|
I think the case of something like Capybara needs to be re-evaluated, as it knows it's going to be running in a context where autoloading would cause a deadlock. I would argue that it needs to call |
This ensures that the "unit of computation" is an entire test, instead of the request. This allows us to skip, for example, connection pooling. This ensures that the same connection is used for the entire test. We should arguably do this for all tests, but this level is the only place that we have an obvious lifecycle to hook into. I think there's still an underlying problem to address, which is that in general we shouldn't release a connection with an open transaction that hasn't been poisoned back into the pool. However, that's a slightly larger change that can be addressed later. Fixes rails#24004 (though this may require an upstream change for RSpec) Fixes rails#23989 Close rails#24491 Close rails#24599
9edc109 to
fe1b500
Compare
|
In either case, a regression test for cf075d9 should be added. |
|
/cc @spastorino re. regression test |
When the query cache completes, if Active Record is still inside of a transaction, it is because the transaction is meant to be left open above this unit of work (such as transactional fixtures in tests). There were several tests around the behavior of "tests" that were invalid, as tests are not run through the executor. They have been changed to reflect the new behavior, which is closer to what actually occurs in Rails tests. Fixes rails#23989 Fixes rails#24491 Close rails#24608 Close rails#24500
|
#24610 is likely a better approach for the long term |
|
👍 will think about how to add a regression test. |
|
In order to test this, you need to change the test like https://github.com/rails/rails/pull/24500/files#diff-4836275ce5b6432a15ca07c1a15593d5L65 I think. |
|
Is this blocking release candidate? Blocking beta4? |
|
It would be nice to address but no, it is not blocking. The visible On Sun, Apr 24, 2016, 3:08 PM Jeremy Daer notifications@github.com wrote:
|
|
Closing in favor of #24610. |
This ensures that the "unit of computation" is an entire test, instead
of the request. This allows us to skip, for example, connection pooling.
This ensures that the same connection is used for the entire test.
We should arguably do this for all tests, but this level is the only
place that we have an obvious lifecycle to hook into.
I think there's still an underlying problem to address, which is that in
general we shouldn't release a connection with an open transaction that
hasn't been poisoned back into the pool. However, that's a slightly
larger change that can be addressed later.
Fixes #24004 (though this may require an upstream change for RSpec)
Fixes #23989
Fixes #24491
Close #24500
Close #24599