-
Notifications
You must be signed in to change notification settings - Fork 122
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
Disable Minitest::Reporters
for RubyMine
#1927
base: main
Are you sure you want to change the base?
Conversation
I think this the git stuff is better suited for people's personal |
cea2318
to
0dcfd99
Compare
.gitignore
Outdated
# Reference: https://intellij-support.jetbrains.com/hc/en-us/articles/206544839 | ||
|
||
# User-specific stuff | ||
.idea/**/workspace.xml |
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 we'd rather just ignore the whole .idea
folder
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.
Do you think I should do that here at all, or leave it to each dev to configure as @andyw8 suggested?
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 is also what's recommended on https://git-scm.com/docs/gitignore#:~:text=Patterns%20which%20a,is%20used%20instead. )
@@ -21,7 +21,9 @@ | |||
backtrace_filter.add_filter(%r{gems/railties}) | |||
backtrace_filter.add_filter(%r{tapioca/helpers/test/}) | |||
|
|||
Minitest::Reporters.use!(SpecReporter.new(color: true), ENV, backtrace_filter) | |||
unless ENV["RM_INFO"] |
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'd rather not special case RubyMine here. I'd be open to changing the default reporter, though, since our custom reporter doesn't work that well.
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 is the only workaround for now, unless we don't use Minitest::Reporters
at all. :(
https://blog.jetbrains.com/ruby/2021/04/improved-minitest-support-action-required/
This is a really poor DX IMO, so I opened an issue, hopefully they can address it: https://youtrack.jetbrains.com/issue/RUBY-33007/Using-Minitest-should-not-require-changes-to-testhelper.rb
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.
Let's try not using it at all, and see what happens?
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.
Left is without, right is with.
I see 2 slight benefits to using this SpecReporter:
- The passing tests are summarized more tersely with simple green dots (though with less detail, like the name and runtime)
- There's a backtrace filter applied
The stock one is more detailed, and perhaps better overall, but that's subjective
0dcfd99
to
804f4e6
Compare
Minitest::Reporters
Minitest::Reporters
for RubyMine
unless ENV["RM_INFO"] | ||
Minitest::Reporters.use!(SpecReporter.new(color: true), ENV, backtrace_filter) | ||
end |
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.
Can we make this to be just:
unless ENV["RM_INFO"] | |
Minitest::Reporters.use!(SpecReporter.new(color: true), ENV, backtrace_filter) | |
end | |
Minitest.backtrace_filter = backtrace_filter |
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.
Can you explain the reasoning? Do we not want to use SpecReporter
?
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.
Yes, that's what I've been trying to say in the other thread:
I'd be open to changing the default reporter, though, since our custom reporter doesn't work that well.
and
Let's try not using it at all, and see what happens?
Workaround for this issue.