Extract RubyLsp::TestHelper for addons to use
#1833
Conversation
andyw8
added
chore
test/test_case.rb
Outdated
| # NOTE: This module is intended to be used by addons for writing their own tests, so keep that in mind if changing. | ||
|
|
||
| module RubyLsp | ||
| module TestCase |
There was a problem hiding this comment.
I've made this a module rather than a class so that it can used from addons that don't use minitest, such as ruby-lsp-rspec.
There was a problem hiding this comment.
(maybe RubyLsp::TestHelpers would be a better name)
test/test_case.rb
Outdated
| module TestCase | ||
| extend T::Sig | ||
|
|
||
| sig do |
There was a problem hiding this comment.
Should we remove all the Sorbet aspects from this, so that we're not forcing addon authors to use it?
There was a problem hiding this comment.
Actually it doesn't matter, sorbet-runtime will be available already.
andyw8
force-pushed
the
andyw8/extract-ruby-lsp-test-case
branch
from
e8e14ad
to
d6edc59
Compare
lib/ruby_lsp/test_case.rb
Outdated
| # NOTE: This module is intended to be used by addons for writing their own tests, so keep that in mind if changing. | ||
|
|
||
| module RubyLsp | ||
| module TestCase |
There was a problem hiding this comment.
IMO, TestCase should be a class instead of a module. We can rename it to TestHelper if we prefer the module style. But personally I'd prefer it being a class and just inherit it.
There was a problem hiding this comment.
But if it's a class then we couldn't use it for https://github.com/st0012/ruby-lsp-rspec/blob/main/spec/ruby_lsp_rspec_spec.rb, right?
There was a problem hiding this comment.
Hmm that's a good point. Maybe we can learn from what Rails does? ActionDispatch::IntegrationTest can be used as a class, but most of its behaviours are defined in the Behavior module, which is then included by the class. And rspec-rails includes that module to provide the same behaviour as well.
In our case, RubyLsp::TestCase will be used in Ruby LSP itself and ruby-lsp-rspec. But in ruby-lsp-rspec I'd include the module like RubyLsp::TestHelper.
Of course, we can just rename this to RubyLsp::TestHelper for now. It's just one method and I won't insist on having a class atm.
There was a problem hiding this comment.
Yeah, let's keep a module for now since it can be used with any test framework. But since it's a module, I'd vote for renaming it since TestCase is already commonly used for parent classes by ActiveSupport::TestCase and Test::Unit::TestCase.
I agree with TestHelper as suggested by Stan.
andyw8
force-pushed
the
andyw8/extract-ruby-lsp-test-case
branch
from
d6edc59
to
c5f8081
Compare
| @@ -18,6 +18,7 @@ | |||
| $VERBOSE = nil unless ENV["VERBOSE"] || ENV["CI"] | |||
|
|
|||
| require_relative "../lib/ruby_lsp/internal" | |||
| require_relative "../lib/ruby_lsp/test_helper" | |||
There was a problem hiding this comment.
I know it's not related to this PR, but can we please update all of these to use regular requires? The lib directory gets inserted into the $LOAD_PATH a few lines above, there's no need to use relative requires.
There was a problem hiding this comment.
#1834 because there's something I want to discuss about that approach.
andyw8
force-pushed
the
andyw8/extract-ruby-lsp-test-case
branch
2 times, most recently
from
d6a4726
to
dac14e9
Compare
andyw8
changed the title
RubyLsp::TestHelper for addons to use
andyw8
force-pushed
the
andyw8/extract-ruby-lsp-test-case
branch
from
dac14e9
to
fd9bd78
Compare
| end | ||
| def with_server(source = nil, uri = Kernel.URI("file:///fake.rb"), &block) | ||
| server = RubyLsp::Server.new(test_mode: true) | ||
| server.process_message(method: "initialized") |
There was a problem hiding this comment.
I added this as it's needed for some of the ruby-lsp-rails tests. Depending on the request it may not always be needed, but it doesn't seem to have much of an overhead.
This will allow us to use
with_serverin addons, e.g. Shopify/ruby-lsp-rails#301