-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark Bugsnag frames as out of project #497
Conversation
I've fixed the maze tests that this change has broken. |
@@ -14,8 +14,8 @@ Scenario Outline: Stack frames can be removed | |||
And the request used the "Ruby Bugsnag Notifier" notifier | |||
And the request used payload v4 headers | |||
And the request contained the api key "a35a2a72bd230ac0aa0f52715bbdc6aa" | |||
And the "file" of stack frame 0 equals "/usr/src/app/stack_frame_modification/initiators/<initiator>.rb" |
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.
You should instead update the features to raise the exception so that bugsnag doesnt appear in the stacktrace
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 comment is still valid. The tests that dont need it should be set back to stack frame 0
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.
I've updated the scenarios to separate the tests that require this change and those that don't.
Additional maze changes have been made to ensure stacks from different types of handled errors (a string vs a caught exception) are both tested. |
spec/report_spec.rb
Outdated
expect(frame["inProject"]).to be_nil | ||
end | ||
end | ||
expect(bugsnag_count).to be > 0 |
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 should be a specific number rather than > 0
@@ -14,8 +14,8 @@ Scenario Outline: Stack frames can be removed | |||
And the request used the "Ruby Bugsnag Notifier" notifier | |||
And the request used payload v4 headers | |||
And the request contained the api key "a35a2a72bd230ac0aa0f52715bbdc6aa" | |||
And the "file" of stack frame 0 equals "/usr/src/app/stack_frame_modification/initiators/<initiator>.rb" |
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 comment is still valid. The tests that dont need it should be set back to stack frame 0
end | ||
Then(/^the "(.+)" of the top project stackframe equals "(.+)"(?: for request (\d+))?$/) do |element, value, request_index| | ||
stacktrace = read_key_path(find_request(request_index)[:body], 'events.0.exceptions.0.stacktrace') | ||
frame_index = 0 |
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.
I think you can use Array#find_index
to make this more concise (probably something like frame_index = stacktrace.find_index { |frame| frame['file'] !~ %r{lib/bugsnag.*\.rb\z} }
)
@@ -12,4 +12,30 @@ | |||
steps %Q{ | |||
When I set environment variable "#{env_var}" to "#{credentials}@#{current_ip}:#{MOCK_API_PORT}" | |||
} | |||
end | |||
Then(/^the "(.+)" of the top project stackframe equals "(.+)"(?: for request (\d+))?$/) do |element, value, request_index| |
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.
Can we have a blank line between step definitions for organization/easier editor navigation? (between lines 15-16, 28-29)
stacktrace = read_key_path(find_request(request_index)[:body], 'events.0.exceptions.0.stacktrace') | ||
frame_index = 0 | ||
stacktrace.each do |frame, index| | ||
unless /.*lib\/bugsnag.*\.rb/.match(frame["file"]) |
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.
The step definition pattern has "of the top project stackframe", so should this actually check the inProject
value instead?
Otherwise, if checking against this regular expression is correct, should the step definition pattern have something like "of the top non-Bugsnag stackframe" instead?
the "#{element}" of stack frame #{frame_index} equals "#{value}" | ||
} | ||
end | ||
Then(/^the "(.+)" of the top project stackframe equals (\d+)(?: for request (\d+))?$/) do |element, value, request_index| |
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.
I don't think you need 2 very similar step definitions; you can probably get away with using (\d+|(".+"))
(so the double quotes are part of the capture for the string case and you can just have equals #{value}
)
spec/stacktrace_spec.rb
Outdated
(starting_line + 0).to_s => " _a = 1", | ||
(starting_line + 1).to_s => " _b = 2", | ||
(starting_line + 2).to_s => " _c = 3", | ||
(starting_line + 3).to_s => " \"Test\".prepnd \"T\"", |
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 easier to read if you used single quotes on the outside so you don't have to escape the double quotes inside.
And the "lineNumber" of the top non-bugsnag stackframe equals <lineNumber> | ||
|
||
Examples: | ||
| ruby version | lineNumber | |
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.
They are all the same line number because you're using the running the same file, so this lineNumber
column isn't needed
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.
Removed
@@ -67,8 +67,8 @@ Scenario Outline: A handled error can attach metadata in a block | |||
And the event "severity" equals "warning" | |||
And the event "severityReason.type" equals "handledException" | |||
And the exception "errorClass" equals "RuntimeError" | |||
And the "file" of stack frame 0 equals "/usr/src/app/handled/block_metadata.rb" |
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.
It seems like lines 70-71 can remain as is (with "stack frame 0" instead of "the top non-bugsnag stackframe") (similar comment may apply to couple other places)
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.
Removed, and broken the top most scenario out into two separate scenarios for the same reason.
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.
lgtm
| 2.5 | unhandled_before_notify | 21 | | ||
|
||
Scenario Outline: Stack frames can be removed |
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.
Perhaps this Scenario Outline
string should be adjusted as it is identical to the previous scenario outline's string
Goal
To enable easier examination of the Bugsnag reporting process, and why certain events appear as they do.
Design
Rather than dropping frames from
lib/bugsnag
they are now included, but will automatically be marked out of project.Tests
Unit test added to ensure Bugsnag frames are added correctly and
inProject
is not set.Will require #496 to be merged first so that Maze tests can be added (in progress)
Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for: