Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Add prototype for custom test reporter for minitest #3187
Changes from 1 commit
30a66c9
db42d90
9ed9e62
41b1834
61c3339
c71eb8b
b7e065f
d11b8fa
5eeb5fb
3a06df3
dff30fc
9cb83bf
d10ceaa
f8a5bc6
8e5ea7e
88ed768
7761d77
81c776c
0e9c212
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.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, likestart_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 aMinitest::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
.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:
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.
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.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
(orfull_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 namedstart_test
?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.
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?
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:
Minitest::Assertion
failureMinitest::UnexpectedError
, e.g. if something in the test raisesI thought it may be useful to distingish these in the UI, similar to how the progress reporter shows
E
vsF
.