Call Executor#wrap around each test #43550
Conversation
|
I can confirm this fixes the view_component test suite. Thanks for the super quick fix @casperisfine ! |
|
My pleasure, thanks for the quick error report :) |
|
One test in |
casperisfine
force-pushed
the
as-test-case-executor-fix
branch
from
69a9d8f
to
7aa372c
Compare
|
I still have no clue why this test get stuck on CI and not on my machine, so I reverted #43546 for now. |
casperisfine
force-pushed
the
as-test-case-executor-fix
branch
from
b3e700e
to
f359512
Compare
|
Ok, so I figured the cause of the stuck test, it's https://github.com/rails/rails/blob/6f30cc09a7ad21898d856059e498ab9ad2ae64a4/actionview/lib/action_view/cache_expiry.rb that isn't reentrant, and it end up dead locking itself with: lock.with_read_lock { lock.with_write_lock {} } |
casperisfine
force-pushed
the
as-test-case-executor-fix
branch
2 times, most recently
from
94e382f
to
05e6e10
Compare
casperisfine
changed the title
casperisfine
force-pushed
the
as-test-case-executor-fix
branch
2 times, most recently
from
d7ab1ac
to
7f8fe94
Compare
It's `Rails.application.executor.wrap` that is responsible for clearing request/job local state such as `CurrentAttributes`. Instead of including an ad hoc helper to clear `CurrentAttributes` it's better to run the executor so we properly clear other states as well. However it means all executor hooks now need to be re-entrant.
casperisfine
force-pushed
the
as-test-case-executor-fix
branch
from
7f8fe94
to
7b50969
Compare
|
Since this has quite a large impact, I've put it behind a feature flag that's enabled by default for new Rails 7 apps. |
| # This makes test cases behave closer to an actual request or job, several | ||
| # features that are normally disabled in test, such as Active Record query cache | ||
| # and asynchronous queries will then be enabled. | ||
| # Rails.application.config.active_support.executor_around_test_case = true |
There was a problem hiding this comment.
It is missing updating configuring.md to add to document this new options.
Ref: rails#43550 If `executor_around_test_case` is enabled, all hooks must be reentrant. However `allow_concurrency = false` register a `MutexHook` that isn't reentrant. So this commit replace it by a `Monitor` which does allow reentrancy.
Fix: #43546
We do want to run the executor callbacks because they do cleanup various global state at the end of an unit of work (
CurrentAttributes,QueryLogs,QueryCache, etc).However
Executor#wraphas a safety to prevent being called recursively.So we can't use it, otherwise all the integration test helpers such as
get,postetc, won't properly cleanup after themselves.So we introduce an alternative
performmethod, which is just likewrap, but don't prevent re-entrency.cc @manuelpuyol @camertron, I tested this branch against https://github.com/github/view_component, I believe it fixes the bug you reported.