fix(ci): patch run-tests.php getTimer() to return float instead of comma-formatted string#3856
Open
Leiyks wants to merge 5 commits into
Open
fix(ci): patch run-tests.php getTimer() to return float instead of comma-formatted string#3856Leiyks wants to merge 5 commits into
Leiyks wants to merge 5 commits into
Conversation
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: e63b1f6 | Docs | Datadog PR Page | Give us feedback! |
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
3 times, most recently
from
a52010c to
fe39d11
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-05-07 15:15:27 Comparing candidate commit fe39d11 in PR branch Found 3 performance improvements and 5 performance regressions! Performance is the same for 185 metrics, 1 unstable metrics. scenario:BM_TeaSapiSpindown
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching4
scenario:SpanBench/benchOpenTelemetryInteroperability
|
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
2 times, most recently
from
98fc510 to
cb5ca1a
Compare
…= 7.2) Lists tests slower than 10s with their durations at the end of the run. The --show-slow flag only exists in PHP 7.2+, so it's gated by a runtime PHP_VERSION_ID check to avoid breaking PHP 7.0/7.1 jobs. Used to verify the hypothesis that recent CI failures are caused by tests exceeding 1000s under valgrind (triggering the number_format E_WARNING in run-tests.php's record() method).
…lgrind tests PHP's run-tests.php getTimer() returns number_format($elapsed, 4) which produces a comma-formatted string (e.g. "1,500.0000") when a test takes >=1000 seconds. PHP 8.0+ raises E_WARNING when that string is used in += arithmetic inside record(), causing the test runner to abort with exit code 2. Patch getTimer() at build time so it returns a float (via round()) instead.
The CI .base_test before_script runs wait-for-service-ready.sh, but WAIT_FOR only listed test-agent — request-replayer was never gated on, even though it is in the agent_httpbin_service() service block used by test_extension_ci, ASAN test_c, and ASAN test_c with multiple observers. Add a request-replayer case to detect_service_type (probing /replay, a read-only endpoint that returns valid JSON when php -S is up), and add request-replayer:80 to WAIT_FOR for the three affected job blocks. PHP Language Tests is unchanged: it only declares test-agent in services.
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
from
20b64ea to
b356d78
Compare
curl -sf fails on 4xx, which would falsely report not-ready if /replay returns 4xx when no dump exists yet (the natural startup state). Drop -f so any HTTP response proves php -S is up and executing index.php; connection failure remains the only signal of an unhealthy service.
… to 30s Two related changes to characterize the remaining test_extension_ci flake (per-test 300s timeouts on request-replayer-heavy tests): 1. Override DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED=0 only for the valgrind invocation. fdbad29 removed this override from ALL_TEST_ENV_OVERRIDE, so the valgrind run inherited the default (enabled) for the first time. If timeouts disappear, the env var change is the proximate cause. If they persist, the cause is in code paths active regardless of the flag. 2. Bump --show-slow from 10s to 30s so the SLOW TEST SUMMARY stays focused on tests that actually approach the 300s per-test timeout, trimming noise from the long tail of normally-slow tests.
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
1 participant
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
sedpatch to$(BUILD_DIR)/run-tests.phpat build time, replacingnumber_format(withround(insidegetTimer()Why
PHP's
run-tests.phpgetTimer()returnsnumber_format($elapsed, 4), which produces a comma-formatted string (e.g."1,500.0000") when a test takes ≥1000 seconds. PHP 8.0+ raisesE_WARNING: A non-numeric value encounteredwhen that string is used in+=arithmetic insiderecord(). The main test runner process treats this as a fatal worker error and exits with code 2.This started surfacing ~1 week ago because commit fdbad29 moved
DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED=0fromALL_TEST_ENV_OVERRIDE(covers all test targets, including valgrind) intoENV_OVERRIDE(covers only the non-valgrind run). The valgrind run now executes with process tags enabled, making live-debugger and sidecar-related tests 5–20× slower under valgrind's instrumentation, pushing some past the 1000s threshold.How
We already patch
run-tests.phpat build time for an unrelateddl()issue. This adds a second patch in the same$(BUILD_DIR)/run-tests.phpMakefile target:round($elapsed, 4)returns afloat(e.g.1999.5678) instead of a comma-formatted string, which is safe for+=arithmetic. The JUnit XMLtime='...'attribute also benefits —1999.5678is more spec-compliant than1,999.5678.Verified the sed pattern matches both PHP 8.4.19 (line 3441) and PHP 8.5.0 (line 3433).
Testing
test_extension_ci: [8.4]andtest_extension_ci: [8.5]valgrind runs no longer abort withE_WARNING: A non-numeric value encountered in run-tests.php