-
Notifications
You must be signed in to change notification settings - Fork 154
Optimize CI test speed with Minitest parallelization and test infrastructure improvements #2530
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
base: main
Are you sure you want to change the base?
Changes from all commits
6ed49f9
61965cd
255d744
3b667b5
c1be355
01a2f83
9af27f0
f9ba794
554900c
4df283f
5b5b8cf
c91806a
eeb452c
48ca13f
83a9953
75b171c
1191d8a
607c944
2aeff2e
b3b6776
2ecd26d
cebf824
4650d61
9b7a709
c6459df
c49556c
39f3877
ba30894
78c79b1
c476f5d
d9b0ea5
d7f8d36
b38be1b
3c18d9e
b6ff842
111364d
bacf6a6
56a9d21
c039ac8
58eb8fa
a85e195
033c3f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # typed: strict | ||
| # frozen_string_literal: true | ||
|
|
||
| module Tapioca | ||
| module Helpers | ||
| module Test | ||
| # Include this module in test classes that are safe to run in parallel threads. | ||
| # | ||
| # A class is safe when it does NOT use minitest-hooks' `before(:all)` / `after(:all)`, | ||
| # since `parallelize_me!` dispatches individual test methods to the thread pool and | ||
| # bypasses the `with_info_handler` lifecycle that minitest-hooks relies on. | ||
| # | ||
| # Thread count is controlled by the `MT_CPU` environment variable | ||
| # (defaults to `Etc.nprocessors`). | ||
| module Parallel | ||
| class << self | ||
| #: (T::Module[top] base) -> void | ||
| def included(base) | ||
| T.cast(base, T.class_of(Minitest::Test)).parallelize_me! | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ class Dummy < Rails::Application | |
| } | ||
| } | ||
| config.logger = Logger.new('/dev/null') | ||
| config.log_level = :fatal | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a comment to explain that this is a performance optimization? |
||
| end | ||
| # The defaults are loaded with the first two version numbers (e.g. "7.1") | ||
| defaults_version = Rails.gem_version.segments.take(2).join(".") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,31 +4,18 @@ | |
| require "tapioca/internal" | ||
| require "minitest/autorun" | ||
| require "minitest/spec" | ||
| require "minitest/hooks/default" | ||
| require "minitest/hooks" # Changed from default to avoid unnecessary hook registration | ||
| require "rails/test_unit/line_filtering" | ||
|
|
||
| require "tapioca/helpers/test/content" | ||
| require "tapioca/helpers/test/template" | ||
| require "tapioca/helpers/test/isolation" | ||
| require "tapioca/helpers/test/parallel" | ||
| require "dsl_spec_helper" | ||
| require "spec_with_project" | ||
| require "rails_spec_helper" | ||
|
|
||
| require "minitest/reporters" | ||
| require "spec_reporter" | ||
|
|
||
| # Minitest::Reporters currently lacks support for Minitest 6 out of the box | ||
| # but we can register the plugin to use it. | ||
| # Ref: https://github.com/minitest-reporters/minitest-reporters/pull/366#issuecomment-3731951673 | ||
| require "minitest/minitest_reporter_plugin" | ||
| Minitest.register_plugin(:minitest_reporter) | ||
|
|
||
| backtrace_filter = Minitest::ExtensibleBacktraceFilter.default_filter | ||
| backtrace_filter.add_filter(%r{gems/sorbet-runtime}) | ||
| backtrace_filter.add_filter(%r{gems/railties}) | ||
| backtrace_filter.add_filter(%r{tapioca/helpers/test/}) | ||
|
Comment on lines
-26
to
-29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we ok with giving this up? |
||
|
|
||
| Minitest::Reporters.use!(SpecReporter.new(color: true), ENV, backtrace_filter) | ||
| # Use default minitest reporter (faster than SpecReporter) | ||
|
|
||
| require "minitest/mock" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,7 +176,7 @@ def shutdown_client | |
| end | ||
|
|
||
| def wait_until_exists(path) | ||
| Timeout.timeout(4) do | ||
| Timeout.timeout(30) do | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this because we're expecting more CPU contention, now that we might actually be fully saturating multiple threads? |
||
| sleep(0.2) until File.exist?(path) | ||
| end | ||
| rescue Timeout::Error | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Actually, this would still be making syscalls. Wanna use a NullLogger like so?
I'm curious to measure how much it might help