Output for `expect([]).to include have_output /asdf/` not easy to understand

Hi,

I'm now using the include-matcher within Aruba to check if any object matches have_output, have_exit_status et al. Unfortunately the output is not as expected and not very helpful for failures.

Can you give me some hints how to improve the output? The output of the all-matcher is much more helpful and suitable for my use case, where more complex objects than number and string are involved.

Example

require 'aruba/rspec'

RSpec.describe '#have_output' do
  context 'when using include', :type => :aruba do
    before :each do
    run('echo MyTest')
    end

    it { expect(all_commands).to include have_output 'MyTest' }
    it { expect(all_commands).to include have_output 'asdf' }
  end

  context 'when using all' do
    before :each do
    run('echo MyTest')
    end

    it { expect(all_commands).to all have_output 'MyTest' }
    it { expect(all_commands).to all have_output 'asdf' }
  end
end

The output of the "all"-failing example, contains a lot informatin, but also has this nice section about each object checked:

     object at index 0 failed to match:
        expected "MyTest" to have output: "asdf"
# ./tmp/blub3.rb:19:in `block (3 levels) in <top (required)>'

which exactly tells the user what's wrong. For the "include"-failing example, this is not true, here the user gets some weird output which is not that helpful.

Diff:
@@ -1,2 +1,86 @@
-[(have output: "asdf")]
+[#<Aruba::Processes::SpawnProcess:0x00000002041940
+  @cmd="echo MyTest",
+  @duplex=true,
+  @environment=

I can see, that just showing what's not included in a list, works for simple
objects - Numbers, Strings etc. - but fails for more complex ones.

Here's the matcher involved in the example given above. It's also part of this branch.

# @!method have_output
#   This matchers checks if <command> has created output
#
#   @return [TrueClass, FalseClass] The result
#
#     false:
#     * if command has not created output
#     true:
#     * if command created output
#
#   @example Use matcher
#
#     RSpec.describe do
#       it { expect(last_command_started).to have_output }
#     end
RSpec::Matchers.define :have_output do |expected|
  match do |actual|
    @old_actual = actual

    next false unless @old_actual.respond_to? :output

    @announcer ||= Aruba::Announcer.new(
      self,
      :stdout => @announce_stdout,
      :stderr => @announce_stderr,
      :dir    => @announce_dir,
      :cmd    => @announce_cmd,
      :env    => @announce_env
    )

    @old_actual.stop(@announcer) unless @old_actual.stopped?

    @actual = Aruba::Platform.unescape(actual.output.chomp, aruba.config.keep_ansi)

    values_match?(expected, @actual)
  end

  diffable

  description { "have output: #{description_of expected}" }
end

if RSpec::Expectations::Version::STRING >= '3.0'
  RSpec::Matchers.alias_matcher :a_command_having_output, :have_output
end

[maxmeyer]

@myronmarston Or does it make sense, that I recycle the PR #807 for this? But then I will use a private API, correct? Or is it ok, to sub-class builtin matchers?

[JonRowe]

The have_output matcher is expected to be used with IO object so it's possible it doesn't compose well like that, did you try have_output include 'snippet'? or it's alias have_output a_string_including 'snippet'? I think those would work better...

Oh, does rspec have it's own have_output-matcher? Then I've overwritten it with the aruba one.

[JonRowe]

Ah, I might be getting confused with the output_to.stdout matcher...

Is there a gitter room? So we don't litter this issue? Or some irc channel?

[myronmarston]

Is there a gitter room? So we don't litter this issue? Or some irc channel?

No gitter room (although I've thought about trying it out -- haven't taken the leap yet). We do have an IRC channel: #rspec on freenode.

Anyhow, sorry for the delay but I'm taking a look at this for the first time now.

[myronmarston]

OK, I see what's going on now.

The all matcher gets the failure_message of the passed matcher for each unmatched item, and includes them in the message. The include matcher does not. Overall, this makes sense: for all, you're expecting the matcher to match all items, so the number of mismatches should be small (usually one or only a handful). For the include matcher, if the match fails it means every item in the collection does not match, so to mirror the behavior of all we would have to print the failure message of the matcher applied against every item in the collection. The collection could be quite large, which means the failure message could get huge very, very quickly. Consider, for example, a collection of 1000 items: if include failed to match, we'd have to print 1000 failure messages to get output like you get with all. Clearly that's not going to work.

I'm open to ideas of how to make include provide better output if you have any.

Thanks a lot, that you have a look into this. I see where the problem is. Unfortunately my use case is different:

• Few more complex objects (aka started commands) in the collection - I would say normally <= 10
• I need the result of the matcher for each object (command) in the collection which failed to tell the user what went wrong - e.g. the result of have_output in it { expect(all_commands).to include have_output('asdf') }

I understand, that you need to support more use cases than the one given above and my idea is not suitable for large collections. I've no problem adding a matcher to aruba's codebase which works for our own needs. My only concern for this matcher is, that I rely on "private" APIs since I sub-classed All.

Another possibility would be to add that matcher to rspec-expectations's codebase. But you made clear, that there's include and I totally understand that you don't want to have two different matchers doing the same, but only differ in output. I added the current implementation of the matcher below:

require 'rspec/matchers/built_in/all'

module Aruba
  module Matchers
    # @api private
    # Provides the implementation for `include_an_object`.
    # Not intended to be instantiated directly.
    class IncludeAnObject < RSpec::Matchers::BuiltIn::All
      protected

      attr_reader :matcher, :failed_objects
      attr_accessor :any_succeeded_object

      public

      def initialize(matcher)
        @matcher              = matcher
        @failed_objects       = {}
        @any_succeeded_object = false
      end

      # @api private
      # @return [String]
      def description
        improve_hash_formatting "include an object #{description_of matcher}"
      end

      def does_not_match?(actual)
        @actual = actual

        return false unless iterable?

        index_objects

        any_succeeded_object == false
      end

      private

      def match(expected, actual)
        @actual   = actual
        @expected = expected

        return false unless iterable?

        index_objects

        any_succeeded_object == true
      end

      def index_objects
        actual.each_with_index do |actual_item, index|
          cloned_matcher = matcher.clone
          begin
            matches = cloned_matcher.matches?(actual_item)
          rescue StandardError
            matches = nil
          end

          if matches
            self.any_succeeded_object = true
            break
          else
            failed_objects[index] = cloned_matcher.failure_message
          end
        end
      end
    end
  end
end

module RSpec
  module Matchers
    # Passes if the provided matcher passes when checked against any
    # element of the collection.
    #
    # @example
    #   expect([1, 4, 5]).to include_an_object be_odd
    #
    # @note You can also use this with compound matchers as well.
    #
    # @example
    #   expect([1, 4, 'a']).to include_an_object( be_odd.and be_an(Integer) )
    def include_an_object(expected)
      ::Aruba::Matchers::IncludeAnObject.new(expected)
    end
  end
end

[myronmarston]

I don't think we want to take on that maintenance burden (and the questions from users about why we have two matchers that do the same thing...) so for now I think it's best if you maintain the matcher in your own code base.

My only concern for this matcher is, that I rely on "private" APIs since I sub-classed All.

I think you're going to be better off copying over the bits of All that you need into your matcher class rather than subclassing All. I know it feels a bit distasteful to do that, and feels like a violation of DRY...but the point of DRY is that changing one place (e.g. a base class) allows another place to stay in sync and keep working without also having to be updated to match. In this case, a change in the base class (All) is more likely to break your class since it's a private API and we reserve the right to change it's internal implementation any time, and any behavioral changes in All may not be desirable for your matcher even if it doesn't break it.

Anyhow, closing.

[maxmeyer]

Ok. Then sub-classing BaseMatcher is "not" a good idea either? Does it make sense to re-implement our matcher-class with RSpec::Matchers.define? Is there any other interface we - at aruba - can rely on to implement more complex matchers which is unlikely to break on an rspec-upgrade or is even part of your public API? Or is there any documentation what minimal interface rspec requires for a matcher class?

[myronmarston]

RSpec::Matchers.define is the public API we provide for custom matchers and is the generally recommended way. We don't use it internally (taking a base class approach instead) for performance reasons. The DSL performs slightly slower since it has to re-evaulate the define block every time the matcher is used due to the fact that the block yields the argument passed to the matcher for a particular usage of it. Naturally, that's slower than having a pre-defined class that doesn't have to be re-evaulated each time. The difference is generally pretty negligible but can add up over thousands of uses of a matcher (as happens with RSpec's built-in matchers).

[myronmarston]

And yes, subclassing BaseMatcher isn't a good idea, either. It's not part of our public API either:

rspec-expectations/lib/rspec/matchers/built_in/base_matcher.rb

Lines 4 to 14 in 7f44763

	# @api private
	#
	# Used _internally_ as a base class for matchers that ship with
	# rspec-expectations and rspec-rails.
	#
	# ### Warning:
	#
	# This class is for internal use, and subject to change without notice. We
	# strongly recommend that you do not base your custom matchers on this
	# class. If/when this changes, we will announce it and remove this warning.
	class BaseMatcher

[maxmeyer]

Yeah, I know that comment. That's why I asked you for some guidance and for some information about the minimal interface a suitable class needs to have to work with rspec.

Performance is not my main concern. I'm more interested in having a stable API I can rely on. I don't know the details about how .define works. If implemententing include_an_object is possible and makes sense I can live with that.

If it is not possible/uses to much internals of Rspec: Does it make sense to you, to have a "class" which describes the interface of a valid matcher class for use cases as part of rspec, where using the define-approach is not that suitable? A class which can be sub-classed by others?

I just copied the bits from All and BaseMatcher together:

class MyMatcher
  include RSpec::Matchers::Composable

  attr_reader :actual, :expected, :rescued_exception

  def initialize(expected)
    @expected = expected
  end

  def description
    'description'
  end

  def does_not_match?(_actual)
  end

  def match_unless_raises(*exceptions)
  end

  def matches?(actual)
  end

  def diffable?
  end

  def match_unless_raises(*exceptions)
  end

  def supports_block_expectations?
  end

  def failure_message
  end

  def failure_message_when_negated
  end

  # @api private
  def expects_call_stack_jump?
    false
  end

  # @private
  def expected_formatted
    RSpec::Support::ObjectFormatter.format(@expected)
  end

  # @private
  def actual_formatted
    RSpec::Support::ObjectFormatter.format(@actual)
  end

  # @private
  def self.matcher_name
    @matcher_name ||= underscore(name.split("::").last)
  end

  def self.has_default_failure_messages?(matcher)
  end
end

[myronmarston]

Performance is not my main concern. I'm more interested in having a stable API I can rely on. I don't know the details about how .define works. If implemententing include_an_object is possible and makes sense I can live with that.

You can implement any matcher with the define API. Or you can define a class by hand if you prefer the clarity of that approach. The define block is evaluated in the context of the singleton class of a matcher instance, which means you can do normal method defs (both with def self.foo and def foo) and the method will act just like methods defined in a class.

Does it make sense to you, to have a "class" which describes the interface of a valid matcher class for use cases as part of rspec, where using the define-approach is not that suitable? A class which can be sub-classed by others?

We already have a class which describes the interface of a valid matcher class for our docs:

http://rspec.info/documentation/3.3/rspec-expectations/RSpec/Matchers/MatcherProtocol.html
https://github.com/rspec/rspec-expectations/blob/master/lib/rspec/matchers/matcher_protocol.rb

It doesn't make sense (IMO) for users to use a base class because there's really no need. Matchers are defined by an abstract protocol, not an inheritance hierarchy. Most of the matcher protocol is optional, anyway. As the docs show, there are only two required methods.