houdini icon indicating copy to clipboard operation
houdini copied to clipboard

[FEATURE] Create the `have_published_event` matcher

Open clarissalimab opened this issue 4 years ago • 19 comments

Given a subject, we need a matcher that evaluates if an event was fired under the expected type, object, order, etc.

Reference: ActiveJob matcher.

Criterias

The matcher should have the abilities to:

  • match events by type;
  • match events given an specific object;
  • match multiple events:
    • in sequence;
    • without checking the order;
    • without mattering that other events were fired.

Object event description

An object event description is:

  • an event type of an event as a string, such as 'transaction.created' or
  • an object to match an object event using Rspec matchers. An example is:
{
	id: match_houid('evt'), # the event has a houid starting with 'evt'
	object: { # object changed by the event has the following characteristics
        'fee_total' => { 'cents' => 100, 'currency' => 'usd' }, # It has a
																# fee_total with two fields: cents, which equals 100 and
																# currency, which equals 'usd'
        'gross_amount' => { 'cents' => 500, 'currency' => 'usd' }, # It has a
															# fee_total with two field: cents, which equals 500 and
															# currenty, which equals 'usd'

				# some other field descriptions would go here.
      }
	type: be_instance_of(string) # we don't care what type
}

Methods

  • .have_published_event/ .have_published_events receives one object event description and checks if the event like that was published;

  • .have_published_events_starting_with receives object event description and checks if the given event was the first one published;

  • .have_published_events_ending_with receives object event description and checks if the given event was the last one published;

  • .have_only_published_event/ .have_only_published_events checks if the only events published are the ones described in the expectation

  • .followed_by receives object event description and checks if the given event happened directly followed by the prior event that was provided - must be preceded by a .have_published_events or .have_only_published_events;

  • .and_later allows you to combine multiple sets of expectations in order: a compound expectation

Examples

One event

describe 'an action that fires one event' do
  subject do
    described_class
      .create(
        nonprofit: nonprofit,
        supporter: supporter,
        subtransactions: [subtransaction]
      )
  end

  it 'publishes an event of transaction.created type' do
    is_expected .to have_only_published_event('transaction.created')
  end

  it 'publishes an event that matches the specific object provided' do
    is_expected 
      .to have_published_event({
        'fee_total' => { 'cents' => 100, 'currency' => 'usd' },
        'gross_amount' => { 'cents' => 500, 'currency' => 'usd' },
        'subtransaction' => { ... },
        ...
      })
  end
end

The first test fails if for some reason, more events are fired besides the one described because of the .have_only_published_event method used. The same does not apply to the second test.

Multiple events

In some situations an action fires multiple events. The scenarios that we want to be able to test are:

  • Multiple events are fired starting with some specific event;
  • Multiple events are fired starting with some specific sequence of events;
  • Multiple events are fired ending with some specific event;
  • Multiple events are fired ending with some specific sequence of events;
  • Multiple events are fired with an specific event at some point;
  • Multiple events are fired with a sequence of multiple events.
describe 'an action that fires multiple events' do
  subject do
    described_class
      .create(
        nonprofit: nonprofit,
        supporter: supporter,
        subtransactions: [subtransaction]
      )
  end

  it 'fires one or more events starting with a transaction.created' do
    is_expected .to have_published_events_starting_with('transaction.created')
  end

  it 'fires events starting with transaction.created and later fires payment.created' do
    is_expected 
      .to have_published_events_starting_with('transaction.created')
      .and_later have_published_event('payment.created')
  end

  it 'fires one or more events ending with subtransaction.created' do
    is_expected.to have_published_events_ending_with('subtransaction.created')
  end

  it 'fires events with offline_transaction.created and later fired one or more events ending with  subtransaction.created' do
    is_expected 
      .to have_published_event('offline_transaction.created')
      .and_later have_published_events_ending_with('subtransaction.created')
  end

  it 'fires one or more events with offline_transaction.created event at some point' do
    is_expected 
        .to have_published_event('offline_transaction.created')
  end

  it 'fires transaction.created followed directly by payment.created and later fires subtransaction.created followed directly by offline_transaction.created' do
    is_expected 
      .to have_published_event('transaction.created')
      .followed_by('payment.created')
      .and_later have_published_event('subtransaction.created')
      .followed_by('offline_transaction.created')
  end

  it 'fires ONLY transaction.created, payment.created, subtransaction.created and offline_transaction.created, in that order' do
    is_expected 
        .to have_only_published_events('transaction.created')
        .followed_by('payment.created')
        .followed_by('subtransaction.created')
        .followed_by('offline_transaction.created')
  end

	it 'fires transaction.created and fires supporter.created in any order' do
		is_expected.to have_published_event('transaction.created') # order doesn't matter so these could be reversed

		is_expected.to have_published_event('supporter.created')
	end

	# OR it could be two specs

	it 'fires transaction.created' do
		is_expected.to have_published_event('transaction.created')
	end

	it 'fires supporter.created' do
		is_expected.to have_published_event('supporter.created')
	end
end

clarissalimab avatar Jun 18 '21 19:06 clarissalimab

@clarissalimab this is a really good start! I have a few thoughts/questions you may want to answer:

  • have_published_events of_types: [:payment, :transaction], :regardless_of_order, :and_any_others doesn't work in Ruby. The problem is that non-hash arguments need to be before hash arguments.
  • What happens if the user passes nothing to have_published_events?
  • What happens if the user passes non-hash arguments to have_published_events? Or even just a single non-hash argument?

wwahammy avatar Jun 18 '21 19:06 wwahammy

have_published_events of_types: [:payment, :transaction], :regardless_of_order, :and_any_others doesn't work in Ruby. The problem is that non-hash arguments need to be before hash arguments.

Hmm, I didn't realize that. Changing the order won't look that great. How about the options would come in a hash? For example:

  it 'publishes events in any order and any others' do
    is_expected
      .to have_published_events of_types: [:payment, :transaction],
        options: [:regardless_of_order, :and_any_others]
  end

We could also get rid of the key arguments. For example:

  it 'publishes events in any order and any others' do
    is_expected
      .to have_published_events [:payment, :transaction], :regardless_of_order, :and_any_others
  end

What happens if the user passes nothing to have_published_events?

My thinking is that it shouldn't be allowed. At first I thought that maybe it could just test if any event was fired, but it seems very broad. What do you think?

What happens if the user passes non-hash arguments to have_published_events? Or even just a single non-hash argument?

It could be smart and assume that if it's a symbol, then it's an event type, and if it's a hash that does not have the with_data key, it could assume that it is an entire object event.

clarissalimab avatar Jun 18 '21 21:06 clarissalimab

My thinking is that it shouldn't be allowed. At first I thought that maybe it could just test if any event was fired, but it seems very broad. What do you think?

Let's not allow it for now.

It could be smart and assume that if it's a symbol, then it's an event type, and if it's a hash that does not have the with_data key, it could assume that it is an entire object event.

I'd change the list of events names from a symbol to a string. I think it makes more sense to have it be "payment.created" than :payment_created. I think we should eventually get rid of those symbols from the whole event publishing system but that's a separate issue.

Is there any reason they'd need the with_data key?


Something still feels off about the design though. I'm not quite sure what it is. Is there any way we can use methods on the return of have_published_event to make this feel more natural?

wwahammy avatar Jun 21 '21 17:06 wwahammy

Is there any reason they'd need the with_data key?

It's not needed, I just made it to look like a phrase.

Something still feels off about the design though. I'm not quite sure what it is. Is there any way we can use methods on the return of have_published_event to make this feel more natural?

That's a good idea! I'll try to rewrite it to see.

clarissalimab avatar Jun 21 '21 19:06 clarissalimab

I like where this is a going. There are a few things:

  • first for 'event type', use the "dotted" version of the event type. So, instead of "transaction_created", use "transaction.created".
  • when you use an event type without the verb, i.e. "transaction" instead of "transaction.created", what should that do?
  • let's say I want to know that three particular events happened in order with no events in between and then, any number of other events, and then two particular events, and then any other events. How do I write that?

Some thoughts for handling complex scenarios:

Block method

I'm wondering if there's a way we can use blocks to describe more complex scenarios. Like consider the following:

it { is_expected.to have_published_events('transaction.created', 'payment.created') }

which means verify that a transaction.created event was followed by a payment.created with any other events before or after Here transaction.created and payment.created could be also be expected objects to match against


it { is_expected.to have_published_events_starting_with('transaction.created', 'payment.created') }

which means verify that the first event transaction.created event was followed by a payment.created with no events before


it { is_expected.to have_published_events_ending_with('transaction.created', 'payment.created') }

which means verify that the event transaction.created event was followed by a payment.created with no events after

it { is_expected.to have_published_events_ending_with('transaction.created', 'payment.created') }

which means verify that the first event transaction.created event was followed by the last event payment.created

it {is_expected.to have_published_events('transaction.created') do |event_manager, subject| # you may want the subject in the block
    expect(event_manager.next).to be_event('payment.created') # be_event matches using same logic as have_published_events
    expect(event_manager.next).to be_event('stripe_charge.created')
    # could have other expectations in here
end}

which means verify that an transaction.created occurred followed directly by a payment.created, followed directly by stripe_charge.created with any other events before or after

it {is_expected.to have_published_events('transaction.created') do |event_manager, subject| 
    expect(event_manager.next).to be_event('payment.created') 

end.and have_published_event('stripe_charge.created') # instead of have_published_event you could use have_published_event_starting_with or have_published_event_ending_with
}

which means verify that an transaction.created occurred followed directly by a payment.created and then, at some point, followed by a stripe_charge.created.

it {is_expected.to have_published_events('transaction.created') do |event_manager, subject| 
    expect(event_manager.next).to be_event('payment.created') 
    expect(event_manager.next).to be_event('stripe_charge.created') 
end.and have_published_events('supporter.created') do |event_manager, subject|
   expect(event_manager.next).to be_event('supporter_address.created')
end.and have_published_event('ticket.created')
}

This means:

  1. Verify that an transaction.created occurred
  2. Followed directly by payment.created
  3. Followed directly by stripe_charge.created
  4. Followed at SOME POINT by supporter.created
  5. Followed directly by supporter_address.created
  6. Followed at SOME POINT by `ticket.created

In effect: have_published_event (or have_published_events) means just keep going from the last place we were until you find something you were looking for, with the caveats that have_published_events_starting_with and have_published_events_ending_with means be more specific.

Alternative to the block method

ORRRR maybe there's a way to create methods that allow us to combine them like:

it {is_expected.to have_published_event('transaction.created')
        .followed_by('payment.created') # match on the next immediate event and followed_by uses the same matching as have_published_event
        .followed_by('stripe_charge.created')
        .and have_published_event('supporter.created')
        .followed_by('supporter_address.created')
        .and have_published_event('ticket.created')
        }

This means the same as the previous example.

wwahammy avatar Jul 07 '21 20:07 wwahammy

when you use an event type without the verb, i.e. "transaction" instead of "transaction.created", what should that do?

I think it should fail. We could create a method that only verifies the event type without considering the verb, but I don't know the use case for that.

let's say I want to know that three particular events happened in order with no events in between and then, any number of other events, and then two particular events, and then any other events. How do I write that?

If we followed my original idea, I think this should be in different expectations. It would look like:

describe 'an action that fires multiple events' do
  subject do
    described_class
      .create(
        nonprofit: nonprofit,
        supporter: supporter,
        subtransactions: [subtransaction]
      )
  end

  it 'publishes events in order and any others' do
    is_expected
      .to have_published_events.of_types(['payment.created', 'transaction.updated'])
        .and_any_others
  end

  it 'publishes some particular events in any order along with others' do
    is_expected
      .to have_published_events.of_types(['payment.created', 'transaction.refunded'])
        .in_any_order
        .and_any_others
  end
end

In that case, though, it wouldn't be aware of the chronology of the events, like:

some don't care events > specific events in order > some more don't care events that include specific events.

I like the solutions you thought of for that. The second approach looks more friendly to me.

clarissalimab avatar Jul 14 '21 19:07 clarissalimab

@clarissalimab if we like the second one, please take a crack at fleshing that out and documenting it.

wwahammy avatar Jul 14 '21 20:07 wwahammy

@wwahammy one thing that came to my mind when I was updating the description: by default, we would allow that other events are fired without being described in the tests? What if some unexpected event is being fired?

clarissalimab avatar Jul 15 '21 19:07 clarissalimab

Let's use these methods from my previous idea for this:

  • have_published_events_ending_with -- make sure these are the last events
  • have_published_events_starting_with -- make sure these are the first events

For situations where we want no other events fired other than what's listed... what kind of method name should we use?

wwahammy avatar Jul 15 '21 20:07 wwahammy

@wwahammy hmm, what about .have_only_published_events or .only at the end? Like:

it 'fires transaction.created, payment.created, subtransaction.created and offline_transaction.created in that order' do
    is_expected 
        .to have_only_published_events('transaction.created')
        .followed_by('payment.created')
        .followed_by('subtransaction.created')
        .followed_by('offline_transaction.created')
end

or:

it 'fires transaction.created, payment.created, subtransaction.created and offline_transaction.created in that order' do
    is_expected 
        .to have_published_events('transaction.created')
        .followed_by('payment.created')
        .followed_by('subtransaction.created')
        .followed_by('offline_transaction.created')
        .only
end

clarissalimab avatar Jul 16 '21 15:07 clarissalimab

I think have_only_published_events seems like the best.

wwahammy avatar Jul 16 '21 16:07 wwahammy

@wwahammy I updated the description with your suggestions! :smile_cat:

clarissalimab avatar Jul 16 '21 18:07 clarissalimab

Make sure to document the use of a compound expectation, i.e have_published_events('transaction.created').and have_published_events('payment.created') which means 'transaction.created' was published and at some point in the future, 'payment.created' was published.

wwahammy avatar Jul 16 '21 18:07 wwahammy

@wwahammy I forgot that one, updated again :P

clarissalimab avatar Jul 16 '21 18:07 clarissalimab

@clarissalimab do you think my edits makes sense?

wwahammy avatar Jul 19 '21 21:07 wwahammy

@wwahammy yes, I think so

clarissalimab avatar Jul 20 '21 21:07 clarissalimab

So after, playing a bit with how to implement this, I think we need to change .and to something else. The problem is that .and means something slightly different in RSpec matchers already. I'm thinking we could change it to something like .and_later but maybe there's a better name. It's also possible we could use followed_by for the situation where we need the event to eventually happen but not immediately and then use a different method for what the functionality we've currently documented for followed_by.

What do you think @clarissalimab?

wwahammy avatar Jul 21 '21 16:07 wwahammy

@wwahammy I like .and_later better, .followed_by for me suggests that it happens immediately after

clarissalimab avatar Jul 21 '21 19:07 clarissalimab

Sweet, let's switch it to that in the issue

wwahammy avatar Jul 21 '21 20:07 wwahammy