-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Introduce have_reported_error matcher #2849
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
8b837a8
to
d2d0e51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Rails.error.report(StandardError.new("test error")) | ||
end | ||
|
||
expect(test_block).to have_reported_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suspiciously looks like passing an argument, and like the “value expectations” syntax.
Do you think a proper “this matcher doesn’t work with value expectations “ error will be shown to users on an attempt to use it mistakenly?
test_block = proc { "no error" } | ||
matcher = have_reported_error | ||
|
||
expect(matcher.matches?(test_block)).to be false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to use the purposed matcher’s usage form like:
expect {
expect { “no error reported” }.to have_reported_error
}.to fail_with(/failure message/)
}.to raise_error("unexpected error") | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be tested directly, by adding a spec with expect { } with this matcher.
Rails.error.report(TestError.new("test"), user_id: 123) | ||
end | ||
|
||
expect(test_block).to have_reported_error(TestError).and have_reported_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really pass when two errors were reported and we expect to have those two reported? Does the matcher fail if either of the errors was not reported?
features/matchers/README.md
Outdated
|
||
```ruby | ||
# passes if Rails.error.report was called with any error | ||
expect { Rails.error.report(StandardError.new) }.to have_reported_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a realistic example that a literal ‘Rails.error.report’ will be called? Or it’s rather some application logic that will bubble up the exception to eg a controller filter that would do that?
Would it be reasonable to give such a real-world-like example here, eg (borrowed from Rails guides) have_reported_error { process_service } ?
# expect { "safe code" }.not_to have_reported_error | ||
# | ||
# @param expected_error [Class, Exception, Regexp, Symbol, nil] the expected error to match | ||
def have_reported_error(expected_error = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually provide an alias to accommodate both styles, like ‘send_email’ and ‘have_sent_email’. Would you be opposed to have ‘reports_error’ as an alias?
fixes #2827
rspec-rails is missing support for Rails ErrorReporter, this was introduced to rails in v7 and has been evolving ever since. With my client, we have moved to using ErrorReporter as a unified error reporting interface, so we can easily move from one error tracking software to another with minimal code changes. And we had a need to test this interface with rspec, so we implemented our own matcher to handle this.
I'm suggesting our internal implementation as is. This is probably not suitable as is for this gem, but I'd like to open discussion with this starting point.
Example usage