Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add formatter regression tests rebased #1304

Closed
wants to merge 15 commits into from

2 participants

@myronmarston
Owner

This is #1292 rebased as discussed. Besides the basic rebase I also ammended one of your commits (7e6f273) to make it include a corresponding spec (see 4c43e1c).

I was going to force push this but I'm noticing some spec failures locally that I don't get from the #1282 branch. Not sure why but it's late so I'm just pushing this so I can go to bed.

@JonRowe
Owner

Yeah, I rebased the branch off master myself just now and it doesn't fail... The diff between our rebased branches is literally the one spec and the attr_reader...

@JonRowe
Owner

o_O

@JonRowe
Owner

Ah, got it, my code removes methods and then rebinds them with the adaptor methods on top, and this seems to irrevocably change the LegacyFormatterUsingSubClassing such that it doesn't work for the other specs... A quick dup prevents this but it does highlight a potential issue, this code won't handle multiple instances of the same legacy formatter well..

@JonRowe
Owner

Ping, have you had a chance to look at this today @myronmarston?

@myronmarston
Owner

No, been busy with family stuff yesterday and today. I'm busy most of the rest of the evening, too :(. I'll try to get to this tonight but feel free to do more with it.

@myronmarston
Owner

BTW, it's very unclear to me what your hack/work around is doing and why. Ideally, we'd only have hacks like that in 2.99 (where it'd be temporary) as I'm not totally comfortable with putting hacks like that in rspec master to keep around. Can you write something up explaining why it's needed? Maybe we can come up with an alternate solution that's much less hacky.

@JonRowe
Owner

Yep, it's needed because any formatter that inherits from our code needs the adaptor mixin to translate calls to super. Unfortunately we can't control the inclusion order but we need the mixin to sit on top of our code, before their code, thus it's rebinding their code on top of ours, "controlling" the inclusion order...

Notification flow looks like this.

Reporter
     |
     | sends 3.x notifications
    \|/
LegacyFormatterAdaptor
     |
     | sends 2.x methods / arguments
    \|/
OldFormatter < BaseFormatter
     |
     | sends 2.x methods / arguments to super
    \|/
Mixin
     |
     | sends 3.x methods / arguments to super
    \|/
BaseFormatter
@JonRowe
Owner

BTW I'm thinking it might be an idea to remove this in a "3.1" release, having given people enough warning that this is changing....

@myronmarston
Owner

I get the diagram, but what's different about one of the formatters we added tests for that breaks that flow? That flow worked for other legacy formatters.

BTW I'm thinking it might be an idea to remove this in a "3.1" release, having given people enough warning that this is changing....

That would be a SemVer violation. Intentional breaking changes are only allowed in major releases. So if it's in 3.0, we can't remove it until 4.0 if we're going to be strictly compliant.

We can, of course, make an exception and state loudly that we are SemVer compliant except for this one thing, but I'm not sure that I want to do that.

@JonRowe
Owner

I get the diagram, but what's different about one of the formatters we added tests for that breaks that flow? That flow worked for other legacy formatters.

It only worked for trivial cases. The real formatters use inclusion themselves so you'd end up with an ancestor chain of [Formatter, LegacyMixin, TheirMixin, BaseFormatter]rather than[Formatter, TheirMixin, LegacyMixin, BaseFormatter], this workaround removes and rebinds the methods from TheirMixin (etc etc) and places them ontop of the LegacyMixin's methods thus creating the correct call stack.

That would be a SemVer violation. Intentional breaking changes are only allowed in major releases. So if it's in 3.0, we can't remove it until 4.0 if we're going to be strictly compliant.

Normally I'd agree, but this is code handling something from 2.x as a courtesy, as we agreed we couldn't limit this to 2.99, I think i'd be ok with making an exception here, I don't think it really counts as a SemVer violation as the breaking change is made in 3.x, it's just a safety mechanism being removed...

@myronmarston
Owner

Thanks, I think I understand the problem now. I'll think more about it to see if I can come up with a better solution.

That would be a SemVer violation. Intentional breaking changes are only allowed in major releases. So if it's in 3.0, we can't remove it until 4.0 if we're going to be strictly compliant.

Normally I'd agree, but this is code handling something from 2.x as a courtesy, as we agreed we couldn't limit this to 2.99, I think i'd be ok with making an exception here, I don't think it really counts as a SemVer violation as the breaking change is made in 3.x, it's just a safety mechanism being removed...

So here's an idea: rather than providing support for legacy formatters directly in rspec-core, let's make an external gem called something like rspec-legacy_formatter_support. It will contain all the code to support legacy formatters in RSpec 3. In 2.99, if the user is using a custom formatter, we can issue a deprecation warning telling the user to install and require that gem to continue using the formatter with RSpec 3. Doing so in 2.99 will silence the warning (so they can get to warning-free before upgrading).

With this route, rspec-core can remain pristine without these nasty hacks. We can get as hacky as we need to in rspec-legacy_formatter_support, and it doesn't bother me much since it's in a separate gem. We can actually get even hackier if need be and put formatter-specific solutions in place (e.g. such as for fivemat). There's no need to set a hard deadline of rspec 3.1 then; this gem can keep working throughout all 3.x releases.

Honestly, I can't think of any downsides to the external gem approach. Wish we had thought of it sooner.

Thoughts?

@myronmarston
Owner

Also, one other huge benefit to the external gem approach: we can quickly release new versions as needed for any other formatters if there's community demand.

@myronmarston
Owner

Also (I keep thinking of more benefits...), moving the legacy support into a separate gem will help us ship beta2 faster: we don't need to wait until we settle on solutions for all legacy formatters and can just ship it with the new formatter API, and ship a preliminary version of the external gem. We can iterate on the external gem and release new versions frequently outside of the beta2 -> rc -> final release sequence for RSpec, since it won't need to be versioned with the rspec gems.

@JonRowe
Owner

We could ship beta 2 with this as "experimental" legacy formatter support and then extract it for rc1?

@JonRowe
Owner

(Meaning I certainly won't have time to do the extraction over the next few weeks, but I'm otherwise keen to do it)

@myronmarston

Works for me. I'm leaning towards merging a version of this that doesn't have the crazy method rebinding, though -- so I'll work on prepping this for merge later tonight.

@JonRowe
Owner

Without this rebinding any formatter using inheritance and mixins will be broken.

@myronmarston

So this commit breaks the Fuubar formatter among others. The scenario is failing now because this is printed now:

#<struct RSpec::Core::Notifications::SummaryNotification duration=0.003856, example_count=6, failure_count=3, pending_count=1>

...rather than the typical "Finished in 2.56 seconds".

I'll probably remove this from what I'm prepping to merge.

BTW, it's unclear to me why this is needed. A few days ago we had something that was working with several (but not all) legacy formatters and it didn't have the more recent mother-of-all-hacks so that's what I'm targetting to get prepped to merge.

Owner

Some formatters use summary_line, which is removed from BaseFormatter, and the signature of colourise was changed, so this is needed for those formatters. I'm assuming that Fuubar is failing because of the rebind stuff with this.

Maybe. I find that I can't reason about this very well at all, with the multiple layers going on here, which is one reason I'm advocating the "redefine the classes entirely for legacy formatter use" approach in the discussion of the other PR. It's very easy to reason about.

@myronmarston

Without this rebinding any formatter using inheritance and mixins will be broken.

We'll need a solution in the final rspec 3 release for this, but for beta2 I think it's ok for some legacy formatters to be broken.

@myronmarston

Closing in favor of #1309. I'm not going to delete this branch, though, so the commits can be cherry-picked as needed in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 8, 2014
  1. @myronmarston
  2. @JonRowe @myronmarston

    prevent false double formatters due to deprecation

    JonRowe authored myronmarston committed
  3. @JonRowe @myronmarston

    Allow legacy formatters to access attributes from `initialize`.

    JonRowe authored myronmarston committed
    This is necessary for the Fuubar formatter, among others.
  4. @myronmarston

    Update Fuubar scenario.

    myronmarston authored
  5. @myronmarston

    Remove fivemat scenario.

    myronmarston authored
    We can't support five mat since it's so non-standard and has some hacks.
    
    - `Fivemat.new` doesn't return an instance of `Fivemat` since it's a module:
    
    https://github.com/tpope/fivemat/blob/5586092ae26241484f5e71fd27de350dfff0a1fa/lib/fivemat.rb#L3-L19
    
    - It rebinds a method from the super-superclass:
    
    https://github.com/tpope/fivemat/blob/5586092ae26241484f5e71fd27de350dfff0a1fa/lib/fivemat/rspec.rb#L61-L64
  6. @myronmarston
  7. @myronmarston
  8. @myronmarston
  9. @myronmarston

    Add scenarios for rspec-extra-formatters.

    myronmarston authored
    The tap formatter is failing due to an attempted frozen string mutation.
  10. @JonRowe @myronmarston

    prevent missing constants due

    JonRowe authored myronmarston committed
Commits on Feb 9, 2014
  1. @JonRowe @myronmarston

    more methods for translation

    JonRowe authored myronmarston committed
  2. @JonRowe @myronmarston
  3. @JonRowe @myronmarston

    make our formatters lazily detected

    JonRowe authored myronmarston committed
  4. @JonRowe @myronmarston

    Correctly undefine methods for rebinding

    JonRowe authored myronmarston committed
  5. @JonRowe
This page is out of date. Refresh to see the latest.
View
79 features/formatters/regression_tests.feature
@@ -0,0 +1,79 @@
+Feature: Regression tests for legacy custom formatters
+
+ Background:
+ Given a file named "spec/passing_and_failing_spec.rb" with:
+ """ruby
+ RSpec.describe "Some examples" do
+ it "passes" do
+ expect(1).to eq(1)
+ end
+
+ it "fails" do
+ expect(1).to eq(2)
+ end
+
+ context "nested" do
+ it "passes" do
+ expect(1).to eq(1)
+ end
+
+ it "fails" do
+ expect(1).to eq(2)
+ end
+ end
+ end
+ """
+ And a file named "spec/pending_spec.rb" with:
+ """ruby
+ RSpec.describe "Some pending examples" do
+ context "pending" do
+ it "is reported as pending" do
+ pending { expect(1).to eq(2) }
+ end
+
+ it "is reported as failing" do
+ pending { expect(1).to eq(1) }
+ end
+ end
+ end
+ """
+
+ Scenario: Use fuubar formatter
+ When I run `rspec --format Fuubar`
+ Then the output should contain "Progress: |============"
+ And the output should contain "6 examples, 3 failures, 1 pending"
+ And the output should contain "The Fuubar formatter uses the deprecated formatter interface"
+ But the output should not contain any error backtraces
+
+ Scenario: Use rspec-instafail formatter
+ When I run `rspec --format RSpec::Instafail`
+ Then the output should contain "6 examples, 3 failures, 1 pending"
+ And the output should contain "The RSpec::Instafail formatter uses the deprecated formatter interface"
+ But the output should not contain any error backtraces
+
+ Scenario: Use rspec-extra-formatters JUnit formatter
+ When I run `rspec --require rspec-extra-formatters --format JUnitFormatter`
+ Then the output should contain:
+ """
+ <testsuite errors="0" failures="3" skipped="1" tests="6"
+ """
+ And the output should contain "The JUnitFormatter formatter uses the deprecated formatter interface"
+ But the output should not contain any error backtraces
+
+ @wip @announce
+ Scenario: Use rspec-extra-formatters Tap formatter
+ When I run `rspec --require rspec-extra-formatters --format TapFormatter`
+ Then the output should contain "TAP version 13"
+ And the output should contain "The TapFormatter formatter uses the deprecated formatter interface"
+ But the output should not contain any error backtraces
+
+ @wip @announce
+ Scenario: Use rspec-spinner formatter
+ When I run `rspec --require rspec_spinner --format RspecSpinner::Spinner`
+ Then the output should contain "TBD"
+
+ @wip @announce
+ Scenario: Use nyancat formatter
+ When I run `rspec --format NyanCatFormatter`
+ Then the output should contain "TBD"
+
View
4 features/step_definitions/additional_cli_steps.rb
@@ -41,6 +41,10 @@
expect(normalized_output).to match(regexp(partial_output))
end
+Then /^the output should not contain any error backtraces$/ do
+ step %q{the output should not contain "lib/rspec/core"}
+end
+
# This step can be generalized if it's ever used to test other colors
Then /^the failing example is printed in magenta$/ do
# \e[35m = enable magenta
View
15 lib/rspec/core/formatters.rb
@@ -1,5 +1,3 @@
-require 'rspec/core/formatters/legacy_formatter'
-
# ## Built-in Formatters
#
# * progress (default) - prints dots for passing examples, `F` for failures, `*` for pending
@@ -55,6 +53,7 @@
module RSpec::Core::Formatters
autoload :DocumentationFormatter, 'rspec/core/formatters/documentation_formatter'
autoload :HtmlFormatter, 'rspec/core/formatters/html_formatter'
+ autoload :LegacyFormatter, 'rspec/core/formatters/legacy_formatter'
autoload :ProgressFormatter, 'rspec/core/formatters/progress_formatter'
autoload :JsonFormatter, 'rspec/core/formatters/json_formatter'
@@ -79,13 +78,14 @@ def self.formatters
# @api private
def initialize(reporter)
@formatters = []
+ @formatter_added = false
@reporter = reporter
end
attr_reader :formatters, :reporter
# @api private
def setup_default(output_stream, deprecation_stream)
- if @formatters.empty?
+ unless @formatter_added
add 'progress', output_stream
end
unless @formatters.any? { |formatter| DeprecationFormatter === formatter }
@@ -95,14 +95,17 @@ def setup_default(output_stream, deprecation_stream)
# @api private
def add(formatter_to_use, *paths)
+ @formatter_added = true
formatter_class = find_formatter(formatter_to_use)
- formatter = formatter_class.new(*paths.map {|p| String === p ? file_at(p) : p})
+
+ args = paths.map { |p| String === p ? file_at(p) : p }
if !Loader.formatters[formatter_class].nil?
+ formatter = formatter_class.new(*args)
@reporter.register_listener formatter, *notifications_for(formatter_class)
else
- RSpec.warn_deprecation "The #{formatter.class} formatter uses the deprecated formatter interface.\n Formatter added at: #{::RSpec::CallerFilter.first_non_rspec_line}"
- formatter = LegacyFormatter.new(formatter)
+ RSpec.warn_deprecation "The #{formatter_class} formatter uses the deprecated formatter interface.\n Formatter added at: #{::RSpec::CallerFilter.first_non_rspec_line}"
+ formatter = LegacyFormatter.new(formatter_class, *args)
@reporter.register_listener formatter, *formatter.notifications
end
View
67 lib/rspec/core/formatters/legacy_formatter.rb
@@ -1,4 +1,5 @@
require 'rspec/core/formatters/helpers'
+require 'rspec/core/formatters/base_formatter'
require 'stringio'
module RSpec
@@ -16,6 +17,44 @@ class LegacyFormatter
dump_failures dump_summary seed close stop deprecation deprecation_summary]
module LegacyInterface
+ def self.our_formatters
+ formatters = []
+ formatters << BaseFormatter if defined?(BaseFormatter)
+ formatters << BaseTextFormatter if defined?(BaseTextFormatter)
+ formatters << DeprecationFormatter if defined?(DeprecationFormatter)
+ formatters << DocumentationFormatter if defined?(DocumentationFormatter)
+ formatters << HtmlFormatter if defined?(HtmlFormatter)
+ formatters << JsonFormatter if defined?(JsonFormatter)
+ formatters << ProgressFormatter if defined?(ProgressFormatter)
+ formatters
+ end
+
+ def self.append_features(other)
+ # stash the methods from the legacy formatter that conflict
+ clashing_methods = (self.instance_methods & other.instance_methods).
+ map { |name| [name,other.instance_method(name)] }.
+ reject { |name, meth| our_formatters.include? meth.owner }
+ clashing_methods.each do |name, meth|
+ meth.owner.__send__ :remove_method, name
+ end
+
+ # implement all of our methods
+ super
+
+ # restore the clashing methods on top of ours
+ override_module = Module.new do
+ class << self
+ attr_accessor :__clashing_methods
+ end
+ def self.included(other)
+ __clashing_methods.each do |(name, meth)|
+ other.send :define_method, name, meth
+ end
+ end
+ end
+ override_module.__clashing_methods = clashing_methods
+ other.send :include, override_module
+ end
def start(count)
super Notifications::CountNotification.new(count)
@@ -86,18 +125,38 @@ def close
def stop
super(Notifications::NullNotification) if defined?(super)
end
+
+ def summary_line(example_count, failure_count, pending_count)
+ summary = pluralize(example_count, "example")
+ summary << ", " << pluralize(failure_count, "failure")
+ summary << ", #{pending_count} pending" if pending_count > 0
+ summary
+ end
+
+ def colorise_summary(summary)
+ if failure_count > 0
+ color(summary, RSpec.configuration.failure_color)
+ elsif pending_count > 0
+ color(summary, RSpec.configuration.pending_color)
+ else
+ color(summary, RSpec.configuration.success_color)
+ end
+ end
end
+ # @api private
+ attr_reader :formatter
+
# @api public
#
# @param formatter
- def initialize(oldstyle_formatter)
- @formatter = oldstyle_formatter
- if @formatter.class.ancestors.include?(BaseFormatter)
- @formatter.class.class_eval do
+ def initialize(formatter_class, *args)
+ if formatter_class.ancestors.include?(BaseFormatter)
+ formatter_class.class_eval do
include LegacyInterface
end
end
+ @formatter = formatter_class.new(*args)
end
# @api public
View
8 rspec-core.gemspec
@@ -46,8 +46,14 @@ Gem::Specification.new do |s|
s.add_development_dependency "nokogiri", "1.5.2"
s.add_development_dependency "coderay", "~> 1.0.9"
-
s.add_development_dependency "mocha", "~> 0.13.0"
s.add_development_dependency "rr", "~> 1.0.4"
s.add_development_dependency "flexmock", "~> 0.9.0"
+
+ # For legacy custom formatter regression tests
+ s.add_development_dependency "fuubar", "1.3.2"
+ s.add_development_dependency "nyan-cat-formatter", "0.5.2"
+ s.add_development_dependency "rspec-instafail", "0.2.4"
+ s.add_development_dependency "rspec_spinner", "2.0.0"
+ s.add_development_dependency "rspec-extra-formatters", "1.0.0"
end
View
13 spec/rspec/core/formatters/legacy_formatter_spec.rb
@@ -6,6 +6,19 @@
RSpec.describe RSpec::Core::Formatters::LegacyFormatter do
include FormatterSupport
+ it 'can access attributes provided by base class accessors in #initialize' do
+ klass = Class.new(LegacyFormatterUsingSubClassing.dup) do
+ def initialize(*args)
+ example_count
+ super
+ end
+ end
+
+ config.add_formatter klass
+ expect(config.formatters.first).to be_a(RSpec::Core::Formatters::LegacyFormatter)
+ expect(config.formatters.first.formatter).to be_a(klass)
+ end
+
[OldStyleFormatterExample, LegacyFormatterUsingSubClassing].each do |klass|
describe "#{klass}" do
Something went wrong with that request. Please try again.