-
Notifications
You must be signed in to change notification settings - Fork 183
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
base: main
Are you sure you want to change the base?
Conversation
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
9e0d218
to
93c2d6d
Compare
58ddebf
to
c31bee6
Compare
end | ||
|
||
sig { params(result: Minitest::Result).void } | ||
def record_pass(result) |
There was a problem hiding this comment.
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.
94fb924
to
b8fd1ab
Compare
lib/ruby_lsp/test_reporting.rb
Outdated
extend T::Sig | ||
|
||
sig { params(class_name: String, test_name: String, file: String).void } | ||
def before_test(class_name:, test_name:, file:) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
b8fd1ab
to
30a66c9
Compare
lib/ruby_lsp/test_reporting.rb
Outdated
full_name: full_name, | ||
file: file, | ||
} | ||
puts result.to_json |
There was a problem hiding this comment.
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.
puts result.to_json | |
json_message = result.to_json | |
$stdout.write("Content-Length: #{json_message.bytesize}\r\n\r\n#{json_message}") |
There was a problem hiding this comment.
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?
lib/ruby_lsp/test_reporting.rb
Outdated
extend T::Sig | ||
|
||
sig { params(class_name: String, test_name: String, file: String).void } | ||
def before_test(class_name:, test_name:, file:) |
There was a problem hiding this comment.
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
?
lib/ruby_lsp/test_reporting.rb
Outdated
require "json" | ||
|
||
module RubyLsp | ||
class TestReporting |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
lib/ruby_lsp/test_reporting.rb
Outdated
params( | ||
class_name: String, | ||
test_name: String, | ||
type: T.untyped, # TODO: what type should this be? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
test_name: result.name, | ||
type: result.failure.class.name, | ||
message: result.failure.message, | ||
file: result.source_location[0], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
We could propose adding this in minitest-reporters
.
a82f421
to
c71eb8b
Compare
|
||
sig { params(test: Minitest::Test).returns(String) } | ||
def file_for_class_name(test) | ||
T.must(Kernel.const_source_location(test.class_name)).first |
There was a problem hiding this comment.
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
=========================================
There was a problem hiding this comment.
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.
Addresses #3176
This adds a custom reporter for use with the upcoming changes planned for the Test Explorer UI. It emits the test progress as a series of events, for easy consumption by the VS Code extension.
I have a version for
test/unit
in progress separately.No tests yet, but I will add once things become more concrete.