Skip to content
Merged
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
4 changes: 4 additions & 0 deletions lib/ruby_lsp/requests/code_lens.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class CodeLens < Listener

BASE_COMMAND = T.let((File.exist?("Gemfile.lock") ? "bundle exec ruby" : "ruby") + " -Itest ", String)
ACCESS_MODIFIERS = T.let(["public", "private", "protected"], T::Array[String])
SUPPORTED_TEST_LIBRARIES = T.let(["minitest", "test-unit"], T::Array[String])

sig { override.returns(ResponseType) }
attr_reader :response
Expand Down Expand Up @@ -156,6 +157,9 @@ def merge_response!(other)

sig { params(node: SyntaxTree::Node, name: String, command: String, kind: Symbol).void }
def add_test_code_lens(node, name:, command:, kind:)
# don't add code lenses if the test library is not supported or unknown
return unless SUPPORTED_TEST_LIBRARIES.include?(@test_library)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just an idea: if we split test code lens listeners into 2, like TestCodeLens and BundlerCodeLens, we can skip registering events in TestCodeLens if the test library is not supported. I think this will save us some unnecessary computation.
If it's preferred, now could be a good time for this before we ship ruby-lsp-rails' code lens support.

WDYT? @vinistock @andyw8

Copy link
Contributor

@andyw8 andyw8 Jun 20, 2023

Choose a reason for hiding this comment

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

I think it's not critical for now though, we can bump the ruby-lsp-rails dependency later if we refactor things.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think splitting is worth the complexity yet.


arguments = [
@path,
name,
Expand Down
3 changes: 1 addition & 2 deletions lib/ruby_lsp/requests/support/dependency_detector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ def detected_test_library
elsif direct_dependency?(/^rspec/)
"rspec"
else
warn("WARNING: No test library detected. Assuming minitest.")
"minitest"
"unknown"
end
end

Expand Down
36 changes: 36 additions & 0 deletions test/requests/code_lens_expectations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,42 @@ def test_bar; end
)
end

def test_no_code_lens_for_unknown_test_framework
source = <<~RUBY
class FooTest < Test::Unit::TestCase
def test_bar; end
end
RUBY
uri = "file:///fake.rb"

document = RubyLsp::Document.new(source: source, version: 1, uri: uri)

emitter = RubyLsp::EventEmitter.new
listener = RubyLsp::Requests::CodeLens.new(uri, emitter, @message_queue, "unknown")
emitter.visit(document.tree)
response = listener.response

assert_equal(0, response.size)
Comment on lines +59 to +61
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response = listener.response
assert_equal(0, response.size)
assert_empty(listener.response)

end

def test_no_code_lens_for_rspec
Copy link
Member

Choose a reason for hiding this comment

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

Does this test verify any behaviour that the unknown one doesn't? Do we need both?

source = <<~RUBY
class FooTest < Test::Unit::TestCase
def test_bar; end
end
RUBY
uri = "file:///fake.rb"

document = RubyLsp::Document.new(source: source, version: 1, uri: uri)

emitter = RubyLsp::EventEmitter.new
listener = RubyLsp::Requests::CodeLens.new(uri, emitter, @message_queue, "rspec")
emitter.visit(document.tree)
response = listener.response

assert_equal(0, response.size)
Comment on lines +77 to +79
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response = listener.response
assert_equal(0, response.size)
assert_empty(listener.response)

end

def test_after_request_hook
message_queue = Thread::Queue.new
create_code_lens_hook_class
Expand Down