houdiniproject / houdini

Free and open source fundraising infrastructure for nonprofits and NGOs
https://houdiniproject.org
Other
189 stars 95 forks source link

[FEATURE] Create the `have_published_event` matcher #620

Open clarissalimab opened 3 years ago

clarissalimab commented 3 years ago

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:

Object event description

An object event description 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

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:

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
wwahammy commented 3 years ago

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

clarissalimab commented 3 years ago

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.

wwahammy commented 3 years ago

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?

clarissalimab commented 3 years ago

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.

wwahammy commented 3 years ago

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

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.

clarissalimab commented 3 years ago

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.

wwahammy commented 3 years ago

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

clarissalimab commented 3 years ago

@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?

wwahammy commented 3 years ago

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

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

clarissalimab commented 3 years ago

@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
wwahammy commented 3 years ago

I think have_only_published_events seems like the best.

clarissalimab commented 3 years ago

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

wwahammy commented 3 years ago

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.

clarissalimab commented 3 years ago

@wwahammy I forgot that one, updated again :P

wwahammy commented 3 years ago

@clarissalimab do you think my edits makes sense?

clarissalimab commented 3 years ago

@wwahammy yes, I think so

wwahammy commented 3 years ago

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?

clarissalimab commented 3 years ago

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

wwahammy commented 3 years ago

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