Allow the reporter to send custom events to registered formatters

[JonRowe]

This is an extension of #1866 as part of work for #1851, it allows the reporter to send an arbitrary event to any registered listeners, it won't allow you to send RSpec internal events and creates a CustomNotification for each event. /cc @nyarly

[nyarly]

That's rad - thanks so much

[myronmarston]

I think this should be a set rather than an array, so that NOTIFICATIONS.include? has constant-time complexity (given that's how you use it).

[myronmarston]

Also, when we add new lifecycle events in the future, it'll be easy to forget to add them to this list. Can you think of a way to add some kind of spec that enforces that all lifecycle events are in this list and prevents us from adding a new event w/o adding it here?

[myronmarston]

So I still think this should be a set for better perf, given that this isn't a super small array (e.g. <= 5 items).

Also, I'd still like something that enforces that we keep this up to date. One idea: make notify verify that the given event is included in this list, unless the caller of notify passes a :skip_supported_notification_verification option (or similar...that's kind a long option name). Then from publish it can pass that flag, but for all other notifications it would only be allowed if the event is in this list, which would enforce that this list is always kept up-to-date with all lifecycle events.

Also, should the constant be RESERVED_NOTIFICATIONS or LIFECYCLE_NOTIFICATIONS or something?

[JonRowe]

So I still think this should be a set for better perf, given that this isn't a super small array (e.g. <= 5 items).

I didn't want to use Set because it requires us to require it, but I've realised we already do for the deprecation formatter... I have mixed feelings about that, I know the performance is better but I'd really like to avoid requiring parts of the standard library, it's something that can catch us out.

Also, I'd still like something that enforces that we keep this up to date. One idea: make notify verify that the given event is included in this list, unless the caller of notify passes a :skip_supported_notification_verification option (or similar...that's kind a long option name). Then from publish it can pass that flag, but for all other notifications it would only be allowed if the event is in this list, which would enforce that this list is always kept up-to-date with all lifecycle events.

Personally I'd like to avoid the overhead of having to check each time in notify, but I'd be willing to work up a test to check that they're all included somehow?

Also, should the constant be RESERVED_NOTIFICATIONS or LIFECYCLE_NOTIFICATIONS or something?

I've changed it to RSPEC_NOTIFICATIONS if you think that helps?

[myronmarston]

I didn't want to use Set because it requires us to require it, but I've realised we already do for the deprecation formatter... I have mixed feelings about that, I know the performance is better but I'd really like to avoid requiring parts of the standard library, it's something that can catch us out.

Set is a simple wrapper around Hash which is what really gives constant time inclusion checks, so we can just use a Hash instead (e.g. with a value of true for each key or whatever...the values don't actually matter). We should probably look into doing that for our other uses of Set although that's out of scope for this PR IMO. We may also want to create our own wrapper class that wraps a Hash and provides the tiny bit of the Set interface we actually use.

Personally I'd like to avoid the overhead of having to check each time in notify, but I'd be willing to work up a test to check that they're all included somehow?

A test would be cool but I couldn't think of a way to test that because such a test would need a list of which events to test, but that list itself is what we are actually trying to test. I came up with this, though...it's basically the idea I mentioned before, but implemented in a prepended module that is defined in spec/support/formatter_support so as to only be active in test environments and not add any runtime overhead (besides to rspec-core's spec suite).

if RSpec::Support::RubyFeatures.module_prepends_supported?
  module RSpec::Core
    class Reporter
      module EnforceRSpecNotificationsListComplete
        def notify(event, *args)
          return super if caller_locations(1, 1).first.label =~ /publish/
          return super if RSPEC_NOTIFICATIONS.include?(event)

          raise "#{event.inspect} must be added to `RSPEC_NOTIFICATIONS`"
        end
      end

      prepend EnforceRSpecNotificationsListComplete
    end
  end
end

I've changed it to RSPEC_NOTIFICATIONS if you think that helps?

👍

[JonRowe]

Ok I'll use a set for now and then work on replacing Set in a new PR after this.

I'm happy to use your module prepend idea.

[myronmarston]

I like the idea and the implementation is nice and straightforward.

[JonRowe]

Fixed up

[myronmarston]

raise_error with no args is prone to false positives -- for example, if you typo'd reporter or publish it would never execute the code you are intending to test but the spec would pass. I think we should always pass some arg to raise_error that specifies some aspect of the error that couldn't result from ruby raising a NameError or ArgumentError.

[JonRowe]

Fair enough, I've modified this to check the error.

[myronmarston]

Fixed up

The fixes are an improvement but there are a couple more things I'd like to see before this is merged.

[myronmarston]

This is kinda non-standard for how we've normally done errors. UnsupportedPublish looks like it's a class constant but it's not. Also, using a singleton instance is potentially problematic if the message gets mutated...

What advantage did you see to doing it this way?

BTW, I think it should use ArgumentError, not StandardError as it's for a bad argument, right?

[JonRowe]

I can go back to a bare raise with a string if required, the thinking behind the singleton was just to save allocations.

[myronmarston]

I can go back to a bare raise with a string if required, the thinking behind the singleton was just to save allocations.

I'm not really concerned about excessive allocations here...it's going to be extremely rare that users hit this error.

[myronmarston]

I think it looks weird to not have a blank line between the require and the module def, but since you'll be removing the set require in another PR soon feel free to merge with this as-is :).

[myronmarston]

LGTM, merge when green.

[myronmarston]

Noticed you didn't add a changelog entry for this so I added one in f00b298. Feel free to revise if you have a better description in mind :).

[JonRowe]

Sorry, I did this at RubyConAU so I may have gotten distracted ;)