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

Multithreaded tests vs. testing utils #33146

Closed
palkan opened this Issue Jun 17, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@palkan
Contributor

palkan commented Jun 17, 2018

Parallel testing feature (#31900) provides two implementations: using threads and using processes.

When dealing with threads, we should take care of thread safety. Although Rails codebase itself is a thread-safe some testing tools are not.

And the problem is not only with thread-safety but thread-isolation.

I've started to collect such potentially problematic places, and ended up with the following list:

  • ActionMailer test delivery.

Relies on mail gem TestMailer which stores deliveries in a class variable.

travel_to is not thread-safe.

It's not a problem when using with Minitest (i.e., include ActiveJob::TestHelper): a new test adapter is created in before_setup hook.
However RSpec, for example, provides its own functionality, and it's a common practice to specify :test adapter globally.

It is pretty often used to test custom caching behavior. It's thread-safe, but all threads use the same store, which is not what we expect in multithreaded tests.

We need some test-specific cache with a per-thread store.

Not sure that the list is complete. And, of course, there are many third-party testing tools which do not care about threads.

Unfortunately, just fixing this non-thread-aware tools is not enough: we have system tests which could rely on this non-threaded functionality (e.g., sending emails in a server thread and verifying them in the main thread, time-traveling features, etc.). That's the same problem as with the shared ActiveRecord connection.

That means we need a way to toggle threaded behavior for these tools (and, btw, third-party testing utils too), probably, through a specific hook:

class ActiveSupport::TestCase
  parallelize(workers: 4, with: :threads)

  multithreaded_setup do
    ActionMailer::Base.delivery_method = :threaded_test
  end

  multithreaded_teardown do
    ActionMailer::Base.delivery_method = :test
  end
end

Or vice versa – to add a system tests hook (IMO, a better option):

system_tests_setup do
  ActionMailer::Base.delivery_method = :shared_test
end

I'd like to help with fixing these problems, but first, we should agree on the way to do that.

/cc @eileencodes

@matthewd

This comment has been minimized.

Member

matthewd commented Jun 17, 2018

Isn't this requirement generally addressed by the parallelize_setup / parallelize_teardown hooks, as long as you default the global / main process's configuration to the thread-aware alternative?

I'd be reluctant to add a system tests hook for this, because that feels like it permanently gives up ground I'm not yet ready to yield: they're incompatible with threads at the moment, but not necessarily forever. Defining "running system tests" = "using process isolation" would make that difficult to change in future.

@palkan

This comment has been minimized.

Contributor

palkan commented Jun 17, 2018

Isn't this requirement generally addressed by the parallelize_setup / parallelize_teardown hooks, as long as you default the global / main process's configuration to the thread-aware alternative?

If we default to the thread-aware implementations then system tests would break, don't they? Or I didn't understand you correctly.

I'd be reluctant to add a system tests hook for this because that feels like it permanently gives up ground I'm not yet ready to yield: they're incompatible with threads at the moment, but not necessarily forever.

Agree. It's too early to give on multithreaded system tests.

As I see, we have three different situations:

  • parallelization with processes – just works, doesn't depend on tests types
  • parallelization with threads + non-system tests – thread-awareness required
  • parallelization with threads + system tests – we need a way to synchronize the current test state with the corresponding requests states running in a separate thread.

The main question is how to properly handle the third case.

Can we run a Capybara server within the same thread as the test itself? Don't think so.

Thus we need some sharable isolated environment. That's possible but, IMO, too specific and complicated.

Anyway, we have plenty of time to figure out the better way.

@matthewd

This comment has been minimized.

Member

matthewd commented Jun 17, 2018

I guess my concern is that the split between 2 & 3 isn't so clean in practice: system tests definitely have multiple threads involved out of the box, but it's not super-improbable for other tests to have threads in flight either.

My current least-bad thought is something that involves hooking Thread.new to track thread parentage, and thus reliably link back to the "test thread". (Thread groups could get us most of the way there, but only if the app / other libraries aren't using them.) Actually overriding the method doesn't scare me much, tbh, but useful tracking gets bumpier when libraries' thread pools become involved.

@palkan

This comment has been minimized.

Contributor

palkan commented Jun 17, 2018

My current least-bad thought is something that involves hooking Thread.new to track thread parentage

ThreadGroup with local variables could help (though we don't have local variables for them, it's easy to add them with a little monkey patch: https://bugs.ruby-lang.org/issues/10658).

...only if the app / other libraries aren't using them.

We can also monkey-patch Thread to check the group variables first and fallback to the thread locals if none exist:

Thread.prepend(Module.new do
  def [](key)
    group[key] || super
  end
end)

Then we only have to initialize the corresponding group variable in parallelize_setup. Thus all thread locals created within a test work without modification, and only locals specified for the group are shared.

@rails-bot rails-bot bot added the stale label Sep 16, 2018

@rails-bot

This comment has been minimized.

rails-bot bot commented Sep 16, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot closed this Sep 23, 2018

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