Formatter regression tests prep for merge

[myronmarston]

This is meant to supersede #1304 and #1292.

• Contains the final form of the cukes, with the failing ones tagged with @wip.
• Contains a couple fixes that originated in Add some regression tests for some popular custom formatter gems. #1292, but now there are specs for the fixes to demonstrate why the fixes are needed and to prevent future regressions.
• I took a slightly different approach to fixing the "progress formatter wrongly added" issue; see d141fca. I think my approach is cleaner/simpler.
• This has been rebased against master.
• In Add some regression tests for some popular custom formatter gems. #1292 and Add formatter regression tests rebased #1304 there were some commits that I couldn't figure out why they were needed (such as c284ecd -- the commit message doesn't explain anything). I didn't include such commits since I only wanted to include stuff that is clearly needed for a reason I could understand.

Even though this doesn't solve all problems with legacy formatters, it's ready to merge and, IMO, it's sufficient for beta2. As @JonRowe and I discussed in #1304, we plan to move the legacy formatter support into an external gem and the work can continue there. What is here is merely a start but contains no complicated hacks.

/cc @JonRowe

[JonRowe]

c284ecd was because I had a circular require issue... autoloading prevented that

[JonRowe]

Looked good to me, I'm ok with the extra hack not being shipped in beta2, although I'd prefer it were, as I think a similar approach is going to be needed in the gem and I'd like earlier feedback...

[myronmarston]

I think a similar approach is going to be needed in the gem and I'd like earlier feedback...

Actually, I just thought of an alternate approach that I think is far simpler and will work better that would be fairly easy to do in the gem: have the gem redefine each of the built-in formatters and base classes with the definition of those classes from 2.99. This is very easy to reason about (if a subclassing custom formatter is subclassing a class that's identically defined, it should work with no additional monkey business needed). There are two slight downsides to this approach:

• It'll only work if the rspec-legacy_formatter_support gem is loaded before the custom formatter (since the 2.99 version of the base class has to be in memory before the custom formatter is loaded and subclasses it). Thus, the deprecation warnings in 2.99 and the README for the gem will have to instruct users to load it by passing a --require rspec/legacy_formatter_support CLI option to ensure it is loaded early.
• Once this gem is loaded, it will not allow a subclassing formatter that assumes the new API to be used. However, I think this is OK: the act of loading this gem signals the user's intent to use a legacy formatter, and I can't think of a time I've ever seen anyone use multiple custom formatters at the same time. The built-in formatters will still work so they can use a built-in one (like progress) with a legacy custom formatter and it'll work just fine.

[JonRowe]

The built-in formatters will still work so they can use a built-in one (like progress) with a legacy custom formatter and it'll work just fine.

As it stands they'll break when redefined.

[myronmarston]

As it stands they'll break when redefined.

Maybe I'm misunderstanding something, but with how I'm imagining this to work (but perhaps have explained poorly), they shouldn't. I'm thinking the gem will redefine the built-in formatters as well, because some custom formatters subclass a non-base class formatter (such as ProgressFormatter) so we can't only redefine the base classes -- we have to redefine all the formatter classes from 2.x that a custom formatter might subclass. Also, when we do the redefinition, I'm imagining that we won't simply re-open the class and redefine the methods; we'll remove the old constant and define a new class that uses the same constant name. That way, there won't be any "leakage" of the definitions of rspec-core's 3.x version of the formatters -- it'll just use the 2.x versions, wrapped by the LegacyFormatterAdapter.

[JonRowe]

We'll have to go and undefine them as compatible formatters, as it stands RSpec will see one of our formatters and attempt to use them as a 3.x version, except they'll be a 2.x version, and will break.

[myronmarston]

We'll have to go and undefine them as compatible formatters, as it stands RSpec will see one of our formatters and attempt to use them as a 3.x version, except they'll be a 2.x version, and will break.

Sure, that make sense. I hadn't considered it.

So yeah, there are some details to work out but what do you think of the idea as a whole?

[JonRowe]

I'm fine with it, but we need a big disclaimer telling people that's what's happening, so they don't expect / attempt to use newer features when the gem's in use.

[maxlinc]

I recommend adding the formatters from parallel_tests to the suite. I found an issue with a formatter I'm working on. I won't recommend my formatter for the suite, because it's very non-standard and WIP, but I think parallel_tests is broken for a similar reason. I found that my formatter didn't work with a deep inheritance structure, but if I modified to extend directly from RSpec::Core::Formatters::BaseFormatter (replacing inheritance w/ mixins) then things worked again. The parallel_tests formatters also have a deeper inheritance structure and similar errors.

If you add them to the suite, it works properly with rspec2, but with rspec3 it fails because:

/opt/boxen/rbenv/versions/2.1.0/lib/ruby/gems/2.1.0/gems/parallel_tests-0.16.10/lib/parallel_tests/rspec/logger_base.rb:35:in `close': wrong number of arguments (1 for 0) (ArgumentError)
    from /Users/Thoughtworker/repos/opensource/rspec-core/lib/rspec/core/formatters/legacy_formatter.rb:80:in `close'
    from /Users/Thoughtworker/repos/opensource/rspec-core/lib/rspec/core/formatters/legacy_formatter.rb:293:in `close'
    from /Users/Thoughtworker/repos/opensource/rspec-core/lib/rspec/core/reporter.rb:128:in `block in notify'
...

[JonRowe]

@maxlinc we're taking steps to allow as many legacy formatters to work as possible, but there will be cases that don't work, especially those that don't do standard patterns of implementation, it may be that parallel_tests is one of those.

[myronmarston]

I do think that the legacy formatter support will be much more reliable in the extracted legacy formatter support gem, because being a separate code base that is only loaded if the user is using legacy formatters allows us to use solutions that wouldn't be viable directly in rspec-core (such as redefining the rspec-core base classes to be exactly as they were in 2.14 -- the main problem is the change in base classes, not the change in formatter API -- we have a legacy adapter that translates quite well to address that issue).