fix(ci): update laravel assertions, skip bailout under valgrind, fix grep#3769
Merged
fix(ci): update laravel assertions, skip bailout under valgrind, fix grep#3769
Conversation
…algrind - Laravel Octane/Latest CommonScenariosTest: update two resource name assertions introduced by 1111e80 (unnamed routes now use route URI instead of 'unnamed_route' suffix): simple_view and error routes now produce 'simple_view' and 'error' respectively. - Laravel V4 CommonScenariosTest: update dynamic route resource name assertion for the same reason — Laravel 4.2 has a uri() method so it hits the new elseif branch; expected value is now 'dynamic_route/{param01}/static/{param02?}'. - bailout_double_hook_clear.phpt: add --SKIPIF-- to skip under valgrind (USE_ZEND_ALLOC=0). The test triggers a PHP fatal bailout inside a posthook which bypasses normal span/hook cleanup, causing an expected valgrind LEAKED TEST SUMMARY. The test still runs in the normal (non-valgrind) pass. Pattern matches the guard added in 856068a for background-sender tests.
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: e229ffd | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
The check `! grep -e 'LEAKED TEST SUMMARY'` was too broad: it matched the phrase as a substring anywhere in valgrind-run-tests.out, including inside the PHPINFO section where run-tests.php writes phpinfo() output. Before this fix, the check was masked on PHP 7.0/7.1 because `bailout_double_hook_clear.phpt` leaked under valgrind, causing run-tests.php to exit 1, which short-circuited the && and skipped grep. After adding the SKIPIF to that test, run-tests.php exits 0 and grep runs, finding 6 embedded occurrences that were always there. The actual section header written by run-tests.php is: LEAKED TEST SUMMARY at line start with no leading spaces. The false-positive lines look like: " valgrind LEAKED TEST SUMMARY. The test still runs in the normal" with leading spaces — clearly not the section header. Anchoring with ^ matches only the real header.
Leiyks
changed the title
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
test_web_laravel_octane_latestandtest_web_laravel_42regressions: update resource name assertions to use route URI instead ofunnamed_route(missed by commit1111e8073)test_extension_ci: [7.0/7.1]: skipbailout_double_hook_clear.phptunder valgrind — the bailout path bypasses normal span cleanup, causing expected (non-bug) leakstest_extension_ci: [7.4/8.0]: anchor theLEAKED TEST SUMMARYgrep to line start to avoid false positives from embedded phrases invalgrind-run-tests.outWhy
Laravel regressions (
1111e8073):LaravelIntegration.phpwas updated to use route URI as the resource name, but two test snapshot assertions still expectedunnamed_routein the span name:Laravel/Octane/Latest/CommonScenariosTest.phplines 172, 261Laravel/V4/CommonScenariosTest.phpline 189Bailout valgrind leak (
test_extension_ci: [7.0/7.1]):bailout_double_hook_clear.phpttriggers a PHP fatal bailout inside a posthook. On PHP 7.0/7.1, the bailout unwinds without running normal hook/span cleanup, leaving valgrind-visible allocations. This is expected behavior, not a bug. Same pattern as commit856068ad4.Grep false positive (
test_extension_ci: [7.4/8.0]): Adding the--SKIPIF--changed the test from PASSING to SKIPPED under valgrind. When a test passes, run-tests.php writes nothing about it; when it skips, additional metadata is written tovalgrind-run-tests.outthat contains the phrase" valgrind LEAKED TEST SUMMARY. The test still runs in the normal"(with leading spaces) — not the actual section header. The un-anchored grep'LEAKED TEST SUMMARY'matched this embedded phrase. On 7.0/7.1 on master this was hidden because run-tests.php exited 1 (leaked tests), short-circuiting the&&before grep could run.How
unnamed_route→simple_viewanderrorHomeController@dynamicRoute unnamed_route→HomeController@dynamicRoute dynamic_route/{param01}/static/{param02?}(actual route URI)USE_ZEND_ALLOC=0 && !SKIP_ASAN(valgrind mode, not ASAN), same condition used in856068ad4'LEAKED TEST SUMMARY'→'^LEAKED TEST SUMMARY'to match only the section header at line startTesting
test_web_laravel_octane_latestexpected greentest_web_laravel_42expected greentest_extension_ci: [7.0]expected greentest_extension_ci: [7.1]expected greentest_extension_ci: [7.4]expected greentest_extension_ci: [8.0]expected green