make_parallel_tests_available method is broken and raising warnings

[Justin-W]

• Include how you run your tests and which testing framework or frameworks you are running.
  • we run via rake spec and rspec
• Include the SimpleCov version you are running in your report.
  • simplecov 0.21.2
• Include your ruby -e "puts RUBY_DESCRIPTION".
  • ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-darwin20]
• Please also specify the gem versions of Rails (if applicable).
  • not using Rails
• Include any other coverage gems you may be using and their versions.
  • N/A

After upgrading to simplecov (0.21.2) and ruby 3.1.2, we are seeing a lot of stderr output generated by this line, because the make_parallel_tests_available method is behaving incorrectly due to incorrect assumptions/implementation within the probably_running_parallel_tests? method.

We use both of the ENV vars that the probably_running_parallel_tests? method checks, but not for parallel testing. We do sequential testing, but our tests span processes because we are collecting coverage from CLI commands executed from within the spec examples, and then merging the separate coverage reports after all tests have been run.

The above process worked perfectly with simplecov 0.17.1 and ruby 2.x, but broke in simplecov 0.18.0 (see #853 and #854).

The warnings on stderr from the probably_running_parallel_tests? method are actually causing some of our tests to fail (because there shouldn't be any STDERR output during those tests), in addition to being very noisy. And the warnings are based on bad assumptions which are incorrect in our test context (i.e. that we're using the parallel_tests gem).

[Justin-W]

@PragTob The problem does seem to be related to the 2 methods mentioned above, because when we monkey patch both methods (to do nothing, and to always return false, respectively), the stderr warnings stop, and our tests and coverage both seem to work correctly (using simplecov 0.21.2), just as they did prior to the upgrade.

So regardless of whatever "value add" those 2 methods enable for other test configurations, their current implementations are breaking simplecov for our configuration.

[PragTob]

@Justin-W hi there, as described in the issue referenced atop the code block you are mentioning the value add of these functions is making it work with parallel_tests as we need to do some processing when the last process is out there.

So - pretty much your use case (merging the test results from many different processes). The feature might even work for you (might I do not know your setup) as the main process in your scenario should be the last process alive and could gather it up.

You don't need to use parallel tests to achieve that (I'm assuming that since you're using an env called "PARALLEL_TEST_GROUPS", you could also use SimpleCovs sub-process functionality: https://github.com/simplecov-ruby/simplecov#running-simplecov-against-subprocesses

I'm not sure how to "fix" this - SimpleCov is used in all kings of environments and removing this would break it for a lot of environments. The easiest I can imagine is introducing a config option to not attempt to load parallel_tests so that this doesn't happen.

Alternative fix ideas welcome.

[Justin-W]

Yes, the simplest fix I thought of was to simply update the current simplecov logic to enable some form of "opt out". Probably via some 3rd ENV var.

Our use of the 2 ENV vars currently checked was entirely to enable simplecov coverage report merging. It would be pretty easy in our setup to also set some 3rd ENV var (to disable the problematic warnings) in the same place we set the other 2 ENV vars for simplecov. (And much simpler than monkeypatching simplecov. :) )

The sub-process options you linked also look like they might be a possible way to reimplement our current config, but would require much more effort than adding an ENV var to opt out of the unneeded auto-load. And it doesn't seem at first read like there would be any other benefit to us (beyond what our current implementation already provides) from reimplementing our coverage config to use the linked sub-process config.

And from our point of view, this was a simplecov functional regression. The auto-load behavior and accompanying warning didn't exist in earlier versions of simplecov, where our existing config already worked fine. And while I can understand that the new behavior may be the best default behavior for most simplecov users (you'd certainly have a better perspective on that than us), I think it does make sense to offer an easy-to-apply "opt out" of that particular behavior. In fact, the presence and content of the warning msg itself makes me think you may already have had some idea that an opt out might be needed?

[PragTob]

The warning message is there because I assumed it'd never happen but if it does I wanted there to be a warning. Essentially you are mimicking you are using parallel_tests without using parallel_tests that is an EXTREME edge case. It is so extreme, that even if we were above 1.0 I would never bump a major for it, as this was more like a bugfix, not a known breakage of a supported interface. SimpleCov is used in many different ways, we can't ever account for all of them and might know when/if we're breaking them.

As for the fix, I would not use an ENV but a setting on simplecov itself during SimpleCov's setup.

[Justin-W]

I would never bump a major for it, as this was more like a bugfix, not a known breakage of a supported interface.

I agree. I didn't mean to suggest a major version bump. I just meant it was a regression for us in terms of what previously worked, doesn't anymore. But I understand that we're leveraging simplecov implementation details in our setup.

As for the fix, I would not use an ENV but a setting on simplecov itself during SimpleCov's setup.

That sounds fine to me. Thanks. 👍