Revise testing approach #319
Conversation
test/ruby_lsp_rails/hover_test.rb
Outdated
| @@ -181,6 +167,10 @@ class ActiveRecord::Base | |||
|
|
|||
| def hover_on_source(source, position) | |||
| with_server(source) do |server, uri| | |||
| while RubyLsp::Addon.addons.first.instance_variable_get(:@client).instance_of?(RubyLsp::Rails::NullClient) | |||
There was a problem hiding this comment.
We're doing the same in #303 but this should be cleaned up.
test/ruby_lsp_rails/hover_test.rb
Outdated
| } | ||
|
|
||
| RunnerClient.any_instance.stubs(model: expected_response) | ||
| # TODO: not yet working |
There was a problem hiding this comment.
@vinistock as the server is in a separate process, we can't simply stub schema_dump_path so this is tricky to test. We could have the test modify the app code to include config.active_record.schema_format = :sql, but I'm wondering if it's worth the effort, or if we should just delete this test.
lib/ruby_lsp/ruby_lsp_rails/addon.rb
Outdated
| Thread.new { @client = RunnerClient.create_client } | ||
| runner_thread = Thread.new { @client = RunnerClient.create_client } | ||
| # For tests, we want to wait for the client to be ready | ||
| runner_thread.join if ENV["RAILS_ENV"] == "test" |
There was a problem hiding this comment.
@vinistock is this what you had in mind?
There was a problem hiding this comment.
(it does slow everything down though, so we'll probably need some way to selectively enable it...)
andyw8
force-pushed
the
andyw8/revise-testing-approach
branch
2 times, most recently
from
40a8bef
to
a03f1f4
Compare
lib/ruby_lsp/ruby_lsp_rails/addon.rb
Outdated
| @@ -31,7 +31,7 @@ def activate(global_state, message_queue) | |||
| @global_state = T.let(global_state, T.nilable(RubyLsp::GlobalState)) | |||
| $stderr.puts("Activating Ruby LSP Rails addon v#{VERSION}") | |||
| # Start booting the real client in a background thread. Until this completes, the client will be a NullClient | |||
| Thread.new { @client = RunnerClient.create_client }.join | |||
| Thread.new { @client = RunnerClient.create_client } | |||
| @@ -124,6 +124,8 @@ def baz; end | |||
|
|
|||
| def generate_definitions_for_source(source, position) | |||
| with_server(source) do |server, uri| | |||
| sleep(0.1) while RubyLsp::Addon.addons.first.instance_variable_get(:@client).is_a?(NullClient) | |||
| @@ -238,6 +151,8 @@ class ActiveRecord::Base | |||
|
|
|||
| def hover_on_source(source, position) | |||
| with_server(source) do |server, uri| | |||
| sleep(0.1) while RubyLsp::Addon.addons.first.instance_variable_get(:@client).is_a?(NullClient) | |||
There was a problem hiding this comment.
Same approach as we're using in definition_test, but we should really extract this to provide a better experience for addon developers.
There was a problem hiding this comment.
I agree that we should extract it into a helper. And I also think in that helper we should use addons.find { |addon| addon.name == "Ruby LSP Rails" } instead. I think addons.first will probably break if we someday add another addon to this project's Gemfile as a development dependency?
There was a problem hiding this comment.
Yeah, we can extract that part out too, e.g. RubyLsp::Addon.get(...).
There was a problem hiding this comment.
It does feel like something addons can use during testing 👍
| @@ -146,55 +100,14 @@ class CompositePrimaryKey < ApplicationRecord | |||
|
|
|||
| **product_id**: integer (PK) | |||
|
|
|||
| **note**: string | |||
| **note**: text | |||
There was a problem hiding this comment.
(the stubbed response was incorrect!)
|
|
||
| **created_at**: datetime | ||
|
|
||
| **updated_at**: datetime | ||
| CONTENT | ||
| end | ||
|
|
||
| test "handles `db/structure.sql` instead of `db/schema.rb`" do |
There was a problem hiding this comment.
With the new approach, these two tests would be awkward, and I don't think really provide much value, so I'm proposing we delete them. (If we did want to keep them, we'd should could them to server_test.rb and adapt them).
There was a problem hiding this comment.
Yeah I think server_test.rb will be a better place for these tests. And since we do test against schema_dump_path being unsupported there, I think it's worth keeping these 2 tests there instead of deleting them.
andyw8
force-pushed
the
andyw8/revise-testing-approach
branch
from
a03f1f4
to
a44dc3f
Compare
Closes #317