bugsnag / bugsnag-laravel

BugSnag notifier for the Laravel PHP framework. Monitor and report Laravel errors.
https://docs.bugsnag.com/platforms/php/laravel/
MIT License
876 stars 129 forks source link

Add assertions on breadcrumbs to queue tests #510

Closed imjoehaines closed 1 year ago

imjoehaines commented 1 year ago

Goal

This PR adds assertions to the number of breadcrumbs left in the queue tests. This is important because we rely on the "looping" event to clear breadcrumbs between jobs:

https://github.com/bugsnag/bugsnag-laravel/blob/1dd9fb6a658f59f784657869d0d8028c583a59f0/src/BugsnagServiceProvider.php#L157-L161

In order to make this work consistently across Laravel versions, I've disabled the default query breadcrumbs and left breadcrumbs at various lifecycle points:

Laravel 5.1 doesn't support all of these events, so has different breadcrumbs than the rest of the fixtures

I've also refactored the feature file to simplify the steps where there's a difference between Laravel versions. Instead of:

And on Laravel versions >= 5.2 the event "metaData.job.name" equals "Illuminate\Queue\CallQueuedHandler@call"
And on Laravel versions >= 5.2 the event "metaData.job.queue" equals "default"
And on Laravel versions >= 5.2 the event "metaData.job.attempts" equals 1
And on Laravel versions >= 5.2 the event "metaData.job.connection" equals "database"

You can now do this:

And on Laravel versions >= 5.2:
  """
  the event "metaData.job.name" equals "Illuminate\Queue\CallQueuedHandler@call"
  the event "metaData.job.queue" equals "default"
  the event "metaData.job.attempts" equals 1
  the event "metaData.job.connection" equals 
  """
imjoehaines commented 1 year ago

I think being required to write steps as strings it would be better to write the assertions out as a table, e.g.:

Then the following assertions are true on Laravel versions > 5.2:
    | app.type    | Queue |
    | other.thing | yes      |

I realise this may seem like semantics, but we're already discussing steps having too many sub-steps included for the logic to be clear when reading the feature files. While this is more explicit about the steps being run, it still makes it more difficult to parse when reading, even if it's more flexible.

I'm not sure how to implement that for all three types of steps currently used without having to handle each separately:

  1. the event {string} equals {string} (also "is null")
  2. the event has a {string} breadcrumb named {string}
  3. the event has {int} breadcrumbs
imjoehaines commented 1 year ago

It would be good to have a better way to do this in general though — I think it's actually quite common to have differences across versions but we typically make the assertions looser to fit, e.g.:

https://github.com/bugsnag/bugsnag-laravel/blob/1dd9fb6a658f59f784657869d0d8028c583a59f0/features/unhandled_view.feature#L8-L12

https://github.com/bugsnag/bugsnag-laravel/blob/1dd9fb6a658f59f784657869d0d8028c583a59f0/features/ooms.feature#L10-L12

Having the ability to keep assertions strict by running them conditionally based on the version is really nice IMO and I think it'd be a shame to lose the flexibility of allowing any step to be conditional

It'd be much nicer to e.g. use tags on a step to make it conditional, but cucumber doesn't support that and it doesn't have a skip_this_step (like skip_this_scenario) AFAICT so this was the best I could come up with — I would be very happy to use some other syntax, e.g. cucumber allows * instead of Given/When/Then/And so this would be ideal:

Given something
Then on Laravel versions >= 5.2:
* something
* another thing
* another another thing

but I couldn't figure out how to get that working

imjoehaines commented 1 year ago

@Cawllec how do you feel about this?

| laravel version | step                     |
|          >= 5.2 | the event "a" equals "b" |
|                 | the event "c" equals "d" |
|                 | the event "e" is null    |
|           < 5.2 | the event "a" equals "x" |
|                 | the event "c" equals "y" |
Cawllec commented 1 year ago

TBH I like that even less, but I don't have a proposal that will accomplish the same thing right now. I think that we can use it for now without issue, and I'll create a ticket to review how we're using cucumber/gherkin and if we need to refactor the step layouts as a whole (based on how we want to address it)