-
Notifications
You must be signed in to change notification settings - Fork 117
fix: openentelemetry-auto-laravel threw warning on \Illuminate\Database\Eloquent\Model::destroy called. #390
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
fix: openentelemetry-auto-laravel threw warning on \Illuminate\Database\Eloquent\Model::destroy called. #390
Conversation
The committers listed above are authorized under a signed CLA. |
src/Instrumentation/Laravel/tests/Integration/LaravelInstrumentationTest.php
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
============================================
+ Coverage 82.82% 83.12% +0.29%
Complexity 1948 1948
============================================
Files 142 142
Lines 8142 8143 +1
============================================
+ Hits 6744 6769 +25
+ Misses 1398 1374 -24 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
|
| updated_at DATETIME | ||
| )'); | ||
|
|
||
| $this->router()->get('/eloquent', function () { |
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 any reason to use the HTTP kernel to test model functionality? 🤔
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 for review!
The main reason is to test in the same way as much as possible, because test cases with similar intentions were defined in other cases and added as additional cases, but as you reviewed, the TraceProvider setting is done on the TestCase class, so it seems that the test itself is possible without going through the HTTP kernel.
However, it is defined as an integration test, I personally think the current way makes more sense, since the test is to be conducted in the same way as the actual use case and a database is required in this case.
But a little redundant.
If a better testing method is exists, I will be create a new PR.
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.
Thanks @rozeo - we can split these out later then 👍
see more details: open-telemetry/opentelemetry-php#1630
I checked to see if other functions were also affected, but only this function was affected.
Closes: open-telemetry/opentelemetry-php#1630