Skip to content
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

Avoid loading Rails or ActiveSupport in tests #510

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ gem "sorbet-static-and-runtime", platforms: :ruby
gem "tapioca", "~> 0.13", require: false, platforms: :ruby
gem "psych", "~> 5.1", require: false
gem "rails", "8.0.0.beta1"
gem "test_declarative"

platforms :mingw, :x64_mingw, :mswin, :jruby do
gem "tzinfo"
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ GEM
spoom (>= 1.2.0)
thor (>= 1.2.0)
yard-sorbet
test_declarative (0.0.6)
thor (1.3.2)
timeout (0.4.1)
tzinfo (2.0.6)
Expand Down Expand Up @@ -289,6 +290,7 @@ DEPENDENCIES
sorbet-static-and-runtime
sqlite3
tapioca (~> 0.13)
test_declarative
tzinfo
tzinfo-data

Expand Down
10 changes: 8 additions & 2 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,13 @@ require "rake/testtask"
Rake::TestTask.new(:test) do |t|
t.libs << "test"
t.libs << "lib"
t.test_files = FileList["test/**/*_test.rb"]
t.test_files = FileList["test/**/*_test.rb"] - ["test/ruby_lsp_rails/server_test.rb"]
end

task default: [:"db:setup", :test]
Rake::TestTask.new("test:server") do |t|
t.libs << "test"
t.libs << "lib"
t.test_files = ["test/ruby_lsp_rails/server_test.rb"]
end

task default: [:"db:setup", :test, "test:server"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test if adding a present? in the add-on code breaks when running bundle exec rake?

I think because test:server is last, then the related test helper only gets required at the end, but I'm not 100% sure.

Suggested change
task default: [:"db:setup", :test, "test:server"]
task default: ["db:setup", :test, "test:server"]

1 change: 1 addition & 0 deletions gemfiles/Gemfile-rails-main
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ gem "sorbet-static-and-runtime", platforms: :ruby
gem "tapioca", "~> 0.11", require: false, platforms: :ruby
gem "psych", "~> 5.1", require: false
gem "rails", github: "rails/rails", branch: "main"
gem "test_declarative"

platforms :mingw, :x64_mingw, :mswin, :jruby do
gem "tzinfo"
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/addon_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

module RubyLsp
module Rails
class AddonTest < ActiveSupport::TestCase
class AddonTest < Minitest::Test
test "name returns add-on name" do
addon = Addon.new
assert_equal("Ruby LSP Rails", addon.name)
Expand Down
4 changes: 2 additions & 2 deletions test/ruby_lsp_rails/code_lens_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

module RubyLsp
module Rails
class CodeLensTest < ActiveSupport::TestCase
setup do
class CodeLensTest < Minitest::Test
def setup
GlobalState.any_instance.stubs(:test_library).returns("rails")
@ruby = Gem.win_platform? ? "ruby.exe" : "ruby"
end
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/definition_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

module RubyLsp
module Rails
class DefinitionTest < ActiveSupport::TestCase
class DefinitionTest < Minitest::Test
test "recognizes model callback with multiple symbol arguments" do
response = generate_definitions_for_source(<<~RUBY, { line: 3, character: 18 })
# typed: false
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/document_symbol_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

module RubyLsp
module Rails
class DocumentSymbolTest < ActiveSupport::TestCase
class DocumentSymbolTest < Minitest::Test
test "recognizes Rails Active Support test cases" do
response = generate_document_symbols_for_source(<<~RUBY)
class Test < ActiveSupport::TestCase
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/hover_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

module RubyLsp
module Rails
class HoverTest < ActiveSupport::TestCase
class HoverTest < Minitest::Test
test "hook returns model column information" do
expected_response = {
schema_file: "#{dummy_root}/db/schema.rb",
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/indexing_enhancement_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

module RubyLsp
module Rails
class IndexingEnhancementTest < ActiveSupport::TestCase
class IndexingEnhancementTest < Minitest::Test
class << self
# For these tests, it's convenient to have the index fully populated with Rails information, but we don't have
# to reindex on every single example or that will be too slow
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/launch_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

module RubyLsp
module Rails
class LaunchTest < ActiveSupport::TestCase
class LaunchTest < Minitest::Test
test "launching the client succeeds" do
outgoing_queue = Thread::Queue.new

Expand Down
28 changes: 15 additions & 13 deletions test/ruby_lsp_rails/runner_client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@

module RubyLsp
module Rails
class RunnerClientTest < ActiveSupport::TestCase
setup do
class RunnerClientTest < Minitest::Test
def setup
@outgoing_queue = Thread::Queue.new
@client = T.let(RunnerClient.new(@outgoing_queue), RunnerClient)
@client = T.let(RunnerClient.new(@outgoing_queue), T.nilable(RunnerClient))
end

teardown do
@client.shutdown
def teardown
T.must(@client).shutdown

# On Windows, the server process sometimes takes a lot longer to shutdown and may end up getting force killed,
# which makes this assertion flaky
Expand All @@ -35,13 +35,13 @@ class RunnerClientTest < ActiveSupport::TestCase
["country_id", "integer", nil, false],
["active", "boolean", "1", false],
]
response = T.must(@client.model("User"))
response = T.must(T.must(@client).model("User"))
assert_equal(columns, response.fetch(:columns))
assert_match(%r{db/schema\.rb$}, response.fetch(:schema_file))
end

test "returns nil if the request returns a nil response" do
assert_nil @client.model("ApplicationRecord") # ApplicationRecord is abstract
assert_nil T.must(@client).model("ApplicationRecord") # ApplicationRecord is abstract
end

test "falls back to null client when bin/rails is not found" do
Expand Down Expand Up @@ -106,7 +106,7 @@ class RunnerClientTest < ActiveSupport::TestCase
request_name: "do_something",
id: 5,
)
@client.delegate_notification(server_addon_name: "My Add-on", request_name: "do_something", id: 5)
T.must(@client).delegate_notification(server_addon_name: "My Add-on", request_name: "do_something", id: 5)
end

test "delegate request" do
Expand All @@ -116,7 +116,7 @@ class RunnerClientTest < ActiveSupport::TestCase
request_name: "do_something",
id: 5,
)
@client.delegate_request(server_addon_name: "My Add-on", request_name: "do_something", id: 5)
T.must(@client).delegate_request(server_addon_name: "My Add-on", request_name: "do_something", id: 5)
end

test "server add-ons can log messages with the editor" do
Expand All @@ -133,8 +133,8 @@ def execute(request, params)
end
RUBY

@client.register_server_addon(File.expand_path("server_addon.rb"))
@client.delegate_notification(server_addon_name: "Tapioca", request_name: "dsl")
T.must(@client).register_server_addon(File.expand_path("server_addon.rb"))
T.must(@client).delegate_notification(server_addon_name: "Tapioca", request_name: "dsl")

# Started booting server
pop_log_notification(@outgoing_queue, RubyLsp::Constant::MessageType::LOG)
Expand All @@ -156,8 +156,10 @@ def execute(request, params)
end
end

class NullClientTest < ActiveSupport::TestCase
setup { @client = NullClient.new }
class NullClientTest < Minitest::Test
def setup
@client = NullClient.new
end

test "#shutdown is a no-op" do
assert_nothing_raised { @client.shutdown }
Expand Down
4 changes: 2 additions & 2 deletions test/ruby_lsp_rails/server_test.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
# typed: true
# frozen_string_literal: true

require "test_helper"
require "test_helper_server"
require "ruby_lsp/ruby_lsp_rails/server"

class ServerTest < ActiveSupport::TestCase
setup do
def setup
@stdout = StringIO.new
@server = RubyLsp::Rails::Server.new(stdout: @stdout, override_default_output_device: false)
end
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails/support/location_builder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
module RubyLsp
module Rails
module Support
class LocationBuilderTest < ActiveSupport::TestCase
class LocationBuilderTest < Minitest::Test
test "line_location_from_s raises argument error if invalid string given" do
assert_raises(ArgumentError) { LocationBuilder.line_location_from_s("banana") }
end
Expand Down
2 changes: 1 addition & 1 deletion test/ruby_lsp_rails_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
require "test_helper"

module RubyLsp
class RailsTest < ActiveSupport::TestCase
class RailsTest < Minitest::Test
test "it has a version number" do
assert RubyLsp::Rails::VERSION
end
Expand Down
18 changes: 7 additions & 11 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
# typed: true
# frozen_string_literal: true

# Configure Rails Environment
ENV["RAILS_ENV"] = "test"

require_relative "../test/dummy/config/environment"
ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)]
ActiveRecord::Migrator.migrations_paths << File.expand_path("../db/migrate", __dir__)
require "sorbet-runtime"
require "rails/test_help"
require "mocha/minitest"
require "ruby_lsp/internal"
require "minitest/autorun"
require "test_declarative"
require "mocha/minitest"
require "ruby_lsp/test_helper"
require "ruby_lsp/ruby_lsp_rails/addon"
require "active_support/testing/assertions" # for assert_nothing_raised

if defined?(DEBUGGER__)
DEBUGGER__::CONFIG[:skip_path] =
Expand All @@ -26,10 +21,11 @@
# Tapioca (and thus Spoom) is not available on Windows
end

module ActiveSupport
class TestCase
module Minitest
class Test
extend T::Sig
include RubyLsp::TestHelper
include ActiveSupport::Testing::Assertions # for assert_nothing_raised

def dummy_root
File.expand_path("#{__dir__}/dummy")
Expand Down
11 changes: 11 additions & 0 deletions test/test_helper_server.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# typed: true
# frozen_string_literal: true

# Configure Rails Environment
ENV["RAILS_ENV"] = "test"

require "test_helper"

require_relative "../test/dummy/config/environment"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the key culprit here. I don't even think we need to switch fully to just Minitest.

As long as we require only the parts of Active Support related to testing, we can continue with the same setup.

But we can't load the dummy app for every test because that will load everything and get us into trouble.

ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)]
ActiveRecord::Migrator.migrations_paths << File.expand_path("../db/migrate", __dir__)
Loading