Reorder deprecation_summary to come before dump_summary

[soulcutter]

deprecation_summary can oftentimes be long and the
last thing printed gets most of the user's focus.
It's probably better that the result of the tests
is given primacy.

[myronmarston]

LGTM.

[myronmarston]

Actually, should there be a test that shows that dump_summary comes after deprecation_summary?

[soulcutter]

It is possible to test, but I'm not sure such a test has a lot of value.

What might be useful to capture in a spec is the behavior that the dump_summary shows up in the last 3 lines or something, right? I wouldn't necessarily want to just test the implementation on this one.

[myronmarston]

It is possible to test, but I'm not sure such a test has a lot of value.

I think something like this would have value:

it "dumps the failure summary after the deprecation summary so failures don't scroll off the screen and get missed" do
  formatter = instance_double("RSpec::Core::Formatter::ProgressFormatter")
  reporter.register_listener(formatter, :dump_summary, :deprecation_summary)

  expect(formatter).to receive(:deprecation_summary).ordered
  expect(formatter).to receive(:dump_summary).ordered

  reporter.finish
end

It'll prevent future regressions and, importantly, shows intent: if we ever try changing the order of these, we'll get a failure that tells us exactly why this order was chosen, at which point we can decide if that reason is still valid.

What might be useful to capture in a spec is the behavior that the dump_summary shows up in the last 3 lines or something, right? I wouldn't necessarily want to just test the implementation on this one.

That could work, but sounds more difficult. You'd have to ensure the deprecation summary output was more than 3 lines for it to really demonstrate this, and the 3 line threshold seems kinda arbitrary/brittle.

[JonRowe]

I'd add the spec @myronmarston suggests

[soulcutter]

That's fair. I know the one I was thinking would have been harder to write, but I figured that was how to capture the intent. The description string of the example works, I suppose

[soulcutter]

Thanks for the spec, @myronmarston - all I had to do was find a context for it

[myronmarston]

The description string of the example works, I suppose

This is one of the reasons I like writing doc strings rather than using one-liners or rspec-given. I use it to express intent all over the place :).

LGTM, merge when green.