Issues with stream configuration / deprecation formatter

[myronmarston]

I'm working on #1092 and I've run into a larger issue that I'd like some feedback on before attempting to solve.

When adding a deprecation for --order default (which is --order defined in RSpec 3), get this error:

/Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/formatters/deprecation_formatter.rb:144:in `deprecation_summary': private method `puts' called for nil:NilClass (NoMethodError)
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/formatters/deprecation_formatter.rb:27:in `deprecation_summary'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:127:in `block in notify'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:126:in `each'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:126:in `notify'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:111:in `finish'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:60:in `report'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:25:in `run'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:89:in `run'
    from /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:17:in `block in autorun'

Here's what's going on:

• The reporter and deprecation formatter are initialized lazily here when RSpec.deprecate.
• RSpec::Core::Configuration#output_stream is initially nil; it gets assigned lazily in RSpec::Core::CommandLine#run. The ||= there goes back to this commit, which, unfortunately, lacks a spec.
• Because I am issuing a deprecation warning from option_parser.rb, it is initializing the reporter and deprecation formatter before the output stream has been set, so it is nil and we get the above error.

So, a few things I'm considering:

• I think we should set output_stream to $stdout in RSpec::Core::Configuration#initialize so that it is never nil.
• There's this bifurcation between RSpec::Core::Configuration#output_stream and #error_stream being settable off of the config object and also being set via CommandLine#run. @dchelimsky, do you remember why run accepts those args? Any reason not to remove them and just default the values on the config object and then allow users to overwrite them there? It would simplify this considerably, I think.
• I'm realizing that with how the deprecation formatter works, if can be initialized with the deprecation stream + output stream before the user has had a chance to configure those...which means that if/when they configure those, it'll effectively ignore the user's config. I'm not sure what the solution is here. Any ideas?

/cc @soulcutter @JonRowe @xaviershay @samphippen

[dchelimsky]

I don't recall specifically. Risk of changing it is external runners that rely on that signature.

[myronmarston]

Risk of changing it is external runners that rely on that signature.

Do you know of any external runners? It would be nice to look into them to see how much this might affect users and gem authors.

[dchelimsky]

There are probably more, and I haven't looked into how they work, but here are 3:

https://github.com/grosser/parallel_tests
https://github.com/jstorimer/spin
https://github.com/sandro/specjour

[JonRowe]

Perhaps we should pass in the configuration rather than the output streams directly to the deprecation formatter, and have it fall back to $stderr when nil then it doesn't matter when they are configured.

[xaviershay]

+1 on defaulting output_stream to something other than nil.

[JonRowe]

I'm working something up now

[myronmarston]

Hmm, merging #1130 is causing problems for me now. I've spent the last hour trying to understand why I'm not getting any spec output and I think I just figured it out.

To repro what I'm seeing, checkout 2-99-maintenance and go to this block in option_parser.rb:

rspec-core/lib/rspec/core/option_parser.rb

Lines 65 to 72 in 0c9c2ab

	parser.on('--order TYPE[:SEED]', 'Run examples by the specified order type.',
	' [default] files are ordered based on the underlying file',
	' system\'s order',
	' [rand] randomize the order of files, groups and examples',
	' [random] alias for rand',
	' [random:SEED] e.g. --order random:123') do |o|
	options[:order] = o
	end

...and add a line like RSpec.configuration.reporter. Run your specs and you'll see no spec output. Merely referencing the reporter at that state gets things screwed up.

Here's what's going on:

• configuration.output is nil at that stage since it defaults to nil and CommandLine hasn't set it yet.
• The reporter adds the progress formatter here.
• The progress formatter gets instantiated with a nil argument here (since output is nil at that point).
• The constructor sets the formatter's output to a StringIO object here.
• ...which means no formatter output is printed to $stdout.

So basically...if the reporter is accessed before CommandLine has set the output, we get no formatter output. Reporter is accessed when deprecations are issued. In the option parser, which is executed very early, before the output is set, a deprecation is issued and I'm trying to add another and it causes no output.

This is screwy :(. I need to head to bed now (still didn't make any further progress on the ordering API deprecations; my evening was taken up by tracking down why I wasn't getting spec output!) but I plan to take a stab at implementing some of the things I've outlined above later this weekend. @JonRowe, that may involve reverting the merge of #1130 as I really think we need to take this in a different direction to get this unscrewed. One other idea I have is to implement some other kind of deferred depecation mechanism so that things that need to issue deprecations before everything has been set up don't get things into this bad state. That seems excessively complicated and I hope to fix things up so that that's not necessary. We'll see.

[myronmarston]

Fixed by #1141 and #1142.