add have_sent_analytics_event rspec matcher

[misaka]

This matcher gets (optionally) installed as part of the generator.

[misaka]

As discussed with @duncanjbrown, we're going to try to change this so that instead of pulling this into the generator as a template, the app that wants to use this matcher will have to require it into the spec that uses the matcher.

[misaka]

Roll out BigQuery client code across BAT services (dfe-analytics)

[duncanjbrown]

Looking vg — nits only inline incl one about naming — wdyt?

Should RSpec be nested under Testing? DfE::Analytics::Testing::RSpec is verbose, but definitely not confusing.

What do you think of including matchers in our own test suite without the require_relative?

[duncanjbrown]

Stray comment

[duncanjbrown]

Should we log here, or are all errors definitely to be ignored?

[misaka]

I think they can safely be ignored. I can't see a way to be tripped up by this when using the matcher in your spec. The only way to trip up that comes to mind is that if you're changing the matcher code, you might be a little bit surprised if you have an error in there, but I think that's rare and that if it happens, it won't be hard to debug when you realise that all the event_types returned are nil.

Do you think that's good enough justification to swallow errors here?

[duncanjbrown]

This reads a bit oddly — could it be

expect(:web_request).to have_been_enqueued_for_analytics

or sth?

[misaka]

You've made this comment twice. Just to say I like this suggestion better, and am commenting on the other below. ignore this comment, see the thesis below.

[duncanjbrown]

This reads a bit oddly — could it be

expect_analytics_event_to_have_been_enqueued(:web_request)

or even

expect_analytics_web_request_event_to_have_been_enqueued # etc...

[misaka]

I agree that this reads oddly, I was justifying doing it this way by looking at existing RSpec matchers have_been_enqueued. It seemed to me that this would provide for the least surprise for users ... but then, maybe you're demonstrating that that's not true? What do you think in light of the existing RSpec matchers?

re: expect_analytics_web_request_event_to_have_been_enqueued I did play with having have_sent_request_analytics_event_types and have_sent_entities_analytics_event_types, but that added complexity at a different levels and I just removed them. Rubocop was whinging about having them in the same file as the have_sent_analytics_event_types, and they wouldn't be usable in feature tests such as the ones I was writing for Publish which would expect both web_requests and one of the entity events.

  def then_it_has_sent_analytics_events
    expect([:web_request, :create_entity]).to have_been_enqueued_event_types
  end

However, as we've both thought of it, maybe there's something to it. To make this work, I'd think we'd want:

• expect_analytics_web_request_event_to_have_been_enqueued
• expect_analytics_entity_event_to_have_been_enqueued
• expect_any_analytics_event_to_have_been_enqueued

then perhaps for completeness:

• have_sent_web_request_analytics_events
• have_sent_entity_analytics_events
• have_sent_any_analytics_events

and then maybe we could put these into `lib/dfe/analytics/rspec/matchers', or another helper file, to keep Rubocop from whinging.

Or we could just add documentation for the available event_types and call it a day. 😉 What do you think?

[misaka]

Should RSpec be nested under Testing? DfE::Analytics::Testing::RSpec is verbose, but definitely not confusing.

Ah, I didn't see dfe/analytics/testing. Is the intention for these to be used in using projects? Or only in dfe-analytics? If the former then ya, let's keep them together, but also shouldn't we document those helpers? If the latter, then I wonder why those are in lib and not spec?

What do you think of including matchers in our own test suite without the require_relative?

💯 good call!

[misaka]

What do you think of including matchers in our own test suite without the require_relative?

💯 good call!

Just to mention, I've left the require in the spec files themselves, it seems potentially useful as part of the example of how to use the matchers, which I hope the specs provide.