fix(stacktrace): Do not rely on AbsPath for filtering #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhcarvalho
force-pushed
the
stacktrace
branch
2 times, most recently
from
400af95 to
eb9ffee
Compare
TestContextifyFrames is concerned with answering "given a Frame, can we load matching source code context?" Therefore, it can perform its job independent of Trace/NewStacktrace.
NewStacktrace and ExtractStacktrace (through filterFrames) used to whitelist stack frames of test and example files that are part of sentry-go. That is likely only done to allow for testing. However, by relying on the absolute path of files to perform the filtering, the correct filtering of frames depended on how sentry-go was checked out on disk. If the repository was checked out as part of a traditional GOPATH tree, tests passed. If the repository was outside of GOPATH, on an arbitraly named path (without a github.com/getsentry/ prefix), tests failed. This commit replaces tests from stacktrace_test.go with new tests in stacktrace_external_test.go. The difference being that the new tests are built as a separate package. The separation is achieved by using the ability to suffix the test package name with '_test'. See 'go help test'. This new test structure makes it easier to distinguish stack frames that belong to the sentry-go SDK vs. stack frames that are part of the tests, thus removing the need for path-pattern-based filtering. The new tests make a more in-depth comparison of the stack frames against expected values. Now NewStacktrace and ExtractStacktrace filter out all the frames that are part of the github.com/getsentry/sentry-go package/Module, without looking at AbsPath.
rhcarvalho
force-pushed
the
stacktrace
branch
from
eb9ffee to
13b96ea
Compare
rhcarvalho
commented
Dec 17, 2019
| @@ -3,6 +3,7 @@ linters: | |||
| disable: | |||
| - megacheck | |||
| - stylecheck | |||
| - funlen | |||
There was a problem hiding this comment.
This is a "new" linter since we bumped the version of golangci-lint in #121. While I tend to prefer small functions, we should not be pleasing linters and discussing how many lines is small or not.
In particular, this linter complains about perfectly fine table test functions with a few test cases.
| if strings.Contains(frame.AbsPath, "github.com/getsentry/sentry-go") && | ||
| !isTestFile && | ||
| !isExampleFile { | ||
| if frame.Module == "github.com/getsentry/sentry-go" { |
There was a problem hiding this comment.
It was a bit curious to me that a Frame has both a Module and a Package field. What do they mean exactly? I might have missed it, but it seemed like the Package field is never set. @kamilogorek for help 🆘
There was a problem hiding this comment.
kamilogorek
reviewed
Dec 17, 2019
| func stacktraceDiff(x, y *sentry.Stacktrace) string { | ||
| return cmp.Diff( | ||
| x, y, | ||
| cmpopts.IgnoreFields(sentry.Frame{}, "AbsPath"), |
There was a problem hiding this comment.
Nice, never seen it before
kamilogorek
approved these changes
Dec 17, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #100