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

Add prototype for custom test reporter for minitest #3187

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
95 changes: 95 additions & 0 deletions lib/minitest/reporters/ruby_lsp_reporter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# typed: strict
# frozen_string_literal: true

require "ruby_lsp/test_reporting"

module Minitest
module Reporters
class RubyLspReporter < ::Minitest::Reporters::BaseReporter
extend T::Sig

sig { void }
def initialize
@reporting = T.let(RubyLsp::TestReporting.new, RubyLsp::TestReporting)
super
end

sig { params(test: Minitest::Test).void }
def before_test(test)
@reporting.before_test(
class_name: T.must(test.class.name),
test_name: test.name,
file: file_for_class_name(test),
)
super
end

sig { params(test: Minitest::Test).void }
def after_test(test)
@reporting.after_test(
class_name: T.must(test.class.name),
test_name: test.name,
file: file_for_class_name(test),
)
super
end

sig { params(test: Minitest::Result).void }
def record(test)
super

# This follows the pattern used by Minitest::Reporters::DefaultReporter
on_record(test)
end

sig { params(test: Minitest::Result).void }
def on_record(test)
if test.passed?
record_pass(test)
elsif test.skipped?
record_skip(test)
elsif test.failure
record_fail(test)
end
end

sig { params(result: Minitest::Result).void }
def record_pass(result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some duplication between the various record_ methods, but I want to understand what's actually required before cleaning this up. We'll know better once integrated with the extension.

info = {
test_name: result.name,
class_name: result.klass,
file: result.source_location[0],
}
@reporting.record_pass(**info)
end

sig { params(result: Minitest::Result).void }
def record_skip(result)
info = {
test_name: result.name,
class_name: result.klass,
message: result.failure.message,
file: result.source_location[0],
}
@reporting.record_skip(**info)
end

sig { params(result: Minitest::Result).void }
def record_fail(result)
info = {
class_name: result.klass,
test_name: result.name,
type: result.failure.class.name,
message: result.failure.message,
file: result.source_location[0],
Copy link
Member

Choose a reason for hiding this comment

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

These record events seem to have access to the file path without having to resort to Module.const_source_location. Is there some other event, like start_suite that we can remember the file path ahead of time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a before_suite which takes a Minitest::Reporters::Suite, but that doesn't have file information:

https://github.com/minitest-reporters/minitest-reporters/blob/265ff4b40d5827e84d7e902b808fbee860b61221/lib/minitest/reporters/base_reporter.rb#L3C11-L3C16

We could propose adding this in minitest-reporters.

}
@reporting.record_fail(**info)
end

sig { params(test: Minitest::Test).returns(String) }
def file_for_class_name(test)
T.must(Kernel.const_source_location(test.class_name)).first
Copy link
Member

Choose a reason for hiding this comment

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

Just a headsup that even in minitest/test-unit it's possible to define test classes dynamically, which make their source location path a bit different, like:

class Foo
  class_eval <<~RUBY
    class FooTest < Test::Unit::TestCase
      def test_foo
        puts "========================================="
        puts Kernel.const_source_location("Foo::FooTest")
        puts "========================================="
        fail
      end
    end
  RUBY
end
=========================================
(eval at /Users/hung-wulo/src/github.com/Shopify/rdoc/test/rdoc/test_rdoc_comment.rb:483)
1
=========================================

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed an update to handle that. Still need to think about what we do in the UI.

end
end
end
end
75 changes: 75 additions & 0 deletions lib/ruby_lsp/test_reporting.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# typed: strict
# frozen_string_literal: true

require "json"

module RubyLsp
class TestReporting
Copy link
Member

Choose a reason for hiding this comment

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

This class has no state. Should these just be class methods instead? Also, I think it's clearer to call this a TestReporter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the name.

Also I am now passing in an IO instance to allow for easier testing and flexibility.

extend T::Sig

sig { params(class_name: String, test_name: String, file: String).void }
def before_test(class_name:, test_name:, file:)
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 the methods in this class should just take in id (or full_name) and leave the assembling to each test framework's addons. For example, I may use test example/group's location as the rspec addon's test id first.

Copy link
Member

Choose a reason for hiding this comment

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

What does before_test mean? If this means when we're starting to run it, should this be named start_test?

full_name = "#{class_name}##{test_name}"
result = {
event: "before_test",
full_name: full_name,
file: file,
}
puts result.to_json
Copy link
Member

Choose a reason for hiding this comment

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

Test outputs and messages may include line breaks, so we need to use JSON RPC to be able to parse this accurately from the client side.

Suggested change
puts result.to_json
json_message = result.to_json
$stdout.write("Content-Length: #{json_message.bytesize}\r\n\r\n#{json_message}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we call to to_json it will escape the line breaks.

Re. the client/server split: does this feature need to involve the server at all? If we're only targetting VS Code, then could the extension watch the output directly?

end

sig { params(class_name: String, test_name: String, file: String).void }
def after_test(class_name:, test_name:, file:)
full_name = "#{class_name}##{test_name}"
result = {
event: "after_test",
full_name: full_name,
file: file,
}
puts result.to_json
end

sig { params(class_name: String, test_name: String, file: String).void }
def record_pass(class_name:, test_name:, file:)
full_name = "#{class_name}##{test_name}"
result = {
event: "pass",
full_name: full_name,
file: file,
}
puts result.to_json
end

sig do
params(
class_name: String,
test_name: String,
type: T.untyped, # TODO: what type should this be?
Copy link
Member

Choose a reason for hiding this comment

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

What is type here? What will we use it for in the extension?

Copy link
Contributor Author

@andyw8 andyw8 Feb 18, 2025

Choose a reason for hiding this comment

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

I've seen two variants so far:

  • A Minitest::Assertion failure
  • A Minitest::UnexpectedError, e.g. if something in the test raises

I thought it may be useful to distingish these in the UI, similar to how the progress reporter shows E vs F.

message: String,
file: String,
).void
end
def record_fail(class_name:, test_name:, type:, message:, file:)
result = {
event: "fail",
type: type,
message: message,
class_name: class_name,
test_name: test_name,
file: file,
}
puts result.to_json
end

sig { params(class_name: String, test_name: String, message: T.nilable(String), file: String).void }
def record_skip(class_name:, test_name:, message:, file:)
result = {
event: "skip",
message: message,
classname: class_name,
file: file,
}
puts result.to_json
end
end
end
6 changes: 6 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@
end
Minitest::Reporters.use!(minitest_reporter)

# Temporary for verification
if ENV["RUBY_LSP"]
require "minitest/reporters/ruby_lsp_reporter"
Minitest::Reporters.use!(Minitest::Reporters::RubyLspReporter.new)
end

module Minitest
class Test
extend T::Sig
Expand Down