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

plugin shouldn't changes reporters unless activated #247

Open
zenspider opened this issue Dec 10, 2017 · 6 comments
Open

plugin shouldn't changes reporters unless activated #247

zenspider opened this issue Dec 10, 2017 · 6 comments

Comments

@zenspider
Copy link

Offending code is here:

https://github.com/kern/minitest-reporters/blob/775d1e1072e793e39931340391e0e8f55e45d5e4/lib/minitest/minitest_reporter_plugin.rb#L70-L74

Which makes the wrong assumption that if the gem is installed, it must be used. It should be activated via some mechanism, but NOT through minitest's plugin system. I suggest requires or other config in the test_helper or equivalent.

See jeremyevans/minitest-parallel_fork#3 for another example where this is messing with other plugins.

@os97673
Copy link
Collaborator

os97673 commented Dec 10, 2017

@zenspider please clarify the definition of "activated" here. Is call of Minitest::Reporters.use! enough to consider the gem activated?

@zenspider
Copy link
Author

Yeah. Any user intervention that explicitly says "I want to use this". A flag, env var, or explicit call would be fine. Simply requiring that file isn't enough.

@os97673
Copy link
Collaborator

os97673 commented Dec 23, 2017

@zenspider I've been looking at the code and it looks like the only place where we could do anything with reporters is plugin_minitest_reporter_init() but the only way user can inform us he/she wants to use minitest reporter is either env.var or cli switch. Another option would be to add our reporter with others and do anything in it only if user calls use! but than would will "share" stdout with SummaryReporter and ProgressReporter minitest adds or rails reporter which is added to provide "better" reporter for rails developers.
So, currently plugins have a choice: try to live with (at least two reporters minitest adds) or force user explicitly enable them (cli param or env.var) and fail to provide the functionality w/o this (this could be ok for minitest-reporters, but definitely will not work for rails).
Another problem: every plugin will have to invent its own way to enable itself (perhaps it should be done in a common way by minitest itself).
So, do you still believe that pluging auto-load is a great feature? If yes, could you please help me to understand how Rails are supposed to provide their own default runner w/o forcing user specify it explicitly and how it will work with Rails+another plugin when user wants to use the latter instead of Rails?

@rajsahae
Copy link

I'm not sure if this is helpful or related to the same issue but I found that simply installing this plugin (but not using it) broke the normal ProgressReporter output for Minitest. Somehow the DelegateReporter was not properly calling prerecord on the ProgressReporter. Uninstalling the plugin fixed things. Is this related or should I open a separate ticket?

@os97673
Copy link
Collaborator

os97673 commented Jul 25, 2019

I'm not sure if this is helpful or related to the same issue but I found that simply installing this plugin (but not using it) broke the normal ProgressReporter output for Minitest. Somehow the DelegateReporter was not properly calling prerecord on the ProgressReporter. Uninstalling the plugin fixed things. Is this related or should I open a separate ticket?

I'd suggest to file a separate ticket with the test (pointer to the test repo) to reproduce the problem so I could check if this is the same problem or not.

@barrettkingram
Copy link
Collaborator

This issue hasn't had any activity for some time. Perhaps it's no longer a concern or other plugins have already introduced workarounds. If this behavior is still causing problems, I think there may be some value in reviving the idea in #237. Otherwise, I will close this out.

flavorjones added a commit to sparklemotion/nokogiri that referenced this issue Oct 14, 2023
because even having it in the Gemfile messes with
minitest-parallel_fork, sigh.

For context, see:

- minitest-reporters/minitest-reporters#247
- jeremyevans/minitest-parallel_fork#3
flavorjones added a commit to sparklemotion/nokogiri that referenced this issue Oct 15, 2023
**What problem is this PR intended to solve?**

drop minitest-reporters, because even having it in the Gemfile messes
with
minitest-parallel_fork, sigh.

For context, see:

- minitest-reporters/minitest-reporters#247
- jeremyevans/minitest-parallel_fork#3

Also, avoid psych 5.1.1 because of
ruby/psych#655
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants