-
Notifications
You must be signed in to change notification settings - Fork 167
Add debugging lines to integration tests #3641
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
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3641 +/- ##
==========================================
- Coverage 62.20% 62.11% -0.09%
==========================================
Files 141 141
Lines 13387 13387
Branches 1753 1753
==========================================
- Hits 8327 8315 -12
- Misses 4263 4273 +10
- Partials 797 799 +2 see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
7a0e865 to
04e6500
Compare
3ff0b28 to
e5a6633
Compare
e5a6633 to
600244a
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-02-12 13:31:47 Comparing candidate commit 99ab139 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 189 metrics, 5 unstable metrics. |
| ? 'Skipping on PHP 8.1 non-ZTS because Symfony sends endpoints' | ||
| : (skipLaravel74 | ||
| ? 'Skipping on PHP 7.4 non-ZTS because of Laravel sends endpoints' | ||
| : '')) |
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 this makes much sense. Maybe the problem is that this is annotated with @Order(1), but this is not actually a test class, it's a trait. Maybe the including test also has a test annotated with @Order(1), or doesn't have the @TestMethodOrder annotation.
Anyway, I think this test can just be removed. It's not testing any observable behaviour. What we care about is whether endpoints are in fact send, not whatever internal state \DDTrace\are_endpoints_collected() returns
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 disagree. We need to test that after adding an endpoint \DDTrace\are_endpoints_collected() says so
...c/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/Symfony62Tests.groovy
Outdated
Show resolved
Hide resolved
9eedc2b to
99ab139
Compare
Description
Reviewer checklist