-
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
Changes from 10 commits
1360276
60a8729
179f3be
a99e452
02bf1c9
eb8ae62
59af113
9e14ecf
90594cf
07ea1bf
25b5b94
0449c44
4c5ea1a
3256f77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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. |
||
And the "lineNumber" of stack frame 0 equals <lineNumber> | ||
And the "file" of the top project stackframe equals "/usr/src/app/stack_frame_modification/initiators/<initiator>.rb" | ||
And the "lineNumber" of the top project stackframe equals <lineNumber> | ||
|
||
Examples: | ||
| ruby version | initiator | lineNumber | | ||
|
@@ -51,7 +51,7 @@ Scenario Outline: Stack frames can be marked as in project | |
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" | ||
And the "file" of the top project stackframe equals "/usr/src/app/stack_frame_modification/initiators/<initiator>.rb" | ||
And the event "exceptions.0.stacktrace.0.inProject" is null | ||
And the event "exceptions.0.stacktrace.1.inProject" is true | ||
And the event "exceptions.0.stacktrace.2.inProject" is true | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can use |
||
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 commentThe 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 Otherwise, if checking against this regular expression is correct, should the step definition pattern have something like "of the top non-Bugsnag stackframe" instead? |
||
frame_index = index | ||
break | ||
end | ||
end | ||
steps %Q{ | ||
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 commentThe 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 |
||
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"]) | ||
frame_index = index | ||
break | ||
end | ||
end | ||
steps %Q{ | ||
the "#{element}" of stack frame #{frame_index} equals #{value} | ||
} | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -503,7 +503,12 @@ def gloops | |
|
||
it "does not mark the top-most stacktrace line as inProject if out of project" do | ||
Bugsnag.configuration.project_root = "/Random/location/here" | ||
Bugsnag.notify(BugsnagTestException.new("It crashed")) | ||
|
||
begin | ||
"Test".prepnd "T" | ||
rescue Exception => e | ||
Bugsnag.notify(e) | ||
end | ||
|
||
expect(Bugsnag).to have_sent_notification{ |payload, headers| | ||
exception = get_exception_from_payload(payload) | ||
|
@@ -514,12 +519,17 @@ def gloops | |
|
||
it "marks the top-most stacktrace line as inProject if necessary" do | ||
Bugsnag.configuration.project_root = File.expand_path File.dirname(__FILE__) | ||
Bugsnag.notify(BugsnagTestException.new("It crashed")) | ||
|
||
begin | ||
"Test".prepnd "T" | ||
rescue Exception => e | ||
Bugsnag.notify(e) | ||
end | ||
|
||
expect(Bugsnag).to have_sent_notification{ |payload, headers| | ||
exception = get_exception_from_payload(payload) | ||
expect(exception["stacktrace"].size).to be >= 1 | ||
expect(exception["stacktrace"].first["inProject"]).to eq(true) | ||
expect(exception["stacktrace"][0]["inProject"]).to eq(true) | ||
} | ||
end | ||
|
||
|
@@ -1050,12 +1060,21 @@ def gloops | |
exception = event["exceptions"][0] | ||
expect(exception["errorClass"]).to eq("RuntimeError") | ||
expect(exception["message"]).to eq("'nil' was notified as an exception") | ||
} | ||
end | ||
|
||
stacktrace = exception["stacktrace"][0] | ||
expect(stacktrace["lineNumber"]).to eq(1047) | ||
expect(stacktrace["file"]).to end_with("spec/report_spec.rb") | ||
expect(stacktrace["code"]["1046"]).to eq(" it 'uses an appropriate message if nil is notified' do") | ||
expect(stacktrace["code"]["1047"]).to eq(" Bugsnag.notify(nil)") | ||
it "includes bugsnag lines marked out of project" do | ||
notify_test_exception | ||
expect(Bugsnag).to have_sent_notification{ |payload, headers| | ||
exception = get_exception_from_payload(payload) | ||
bugsnag_count = 0 | ||
exception["stacktrace"].each do |frame| | ||
if /.*lib\/bugsnag.*\.rb/.match(frame["file"]) | ||
bugsnag_count += 1 | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This should be a specific number rather than > 0 |
||
} | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,25 +2,29 @@ | |
|
||
describe Bugsnag::Stacktrace do | ||
it "includes code in the stack trace" do | ||
_a = 1 | ||
_b = 2 | ||
_c = 3 | ||
notify_test_exception | ||
_d = 4 | ||
_e = 5 | ||
_f = 6 | ||
begin | ||
_a = 1 | ||
_b = 2 | ||
_c = 3 | ||
"Test".prepnd "T" | ||
_d = 4 | ||
_e = 5 | ||
_f = 6 | ||
rescue Exception => e | ||
Bugsnag.notify(e) | ||
end | ||
|
||
expect(Bugsnag).to have_sent_notification{ |payload, headers| | ||
exception = get_exception_from_payload(payload) | ||
starting_line = __LINE__ - 10 | ||
expect(exception["stacktrace"][1]["code"]).to eq({ | ||
(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 => " notify_test_exception", | ||
(starting_line + 4).to_s => " _d = 4", | ||
(starting_line + 5).to_s => " _e = 5", | ||
(starting_line + 6).to_s => " _f = 6" | ||
starting_line = __LINE__ - 13 | ||
expect(exception["stacktrace"][0]["code"]).to eq({ | ||
(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 commentThe 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. |
||
(starting_line + 4).to_s => " _d = 4", | ||
(starting_line + 5).to_s => " _e = 5", | ||
(starting_line + 6).to_s => " _f = 6" | ||
}) | ||
} | ||
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.
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.