Skip to content

fix(tests): eliminate uniqid() parallel-flake and PHP 8.5 setAccessible() deprecation#55

Merged
markshust merged 1 commit intodevelopfrom
feature/fix-test-uniqid-flake
May 2, 2026
Merged

fix(tests): eliminate uniqid() parallel-flake and PHP 8.5 setAccessible() deprecation#55
markshust merged 1 commit intodevelopfrom
feature/fix-test-uniqid-flake

Conversation

@markshust
Copy link
Copy Markdown
Collaborator

Summary

Two related test-suite hygiene fixes that surfaced during the 0.5.0 release run:

  • Replace bare uniqid() with bin2hex(random_bytes(8)) across all 54 test files that used it for temp-directory naming. PHP's uniqid() is microtime-based and the docs explicitly warn it doesn't guarantee uniqueness without entropy — when pest runs with --parallel (10 worker processes), two workers can call uniqid() in the same microsecond, get the same string, and race on filesystem state.
  • Remove three ReflectionProperty::setAccessible(true) calls in packages/core/tests/Unit/ApplicationTest.php. Deprecated in PHP 8.5 because reflection on private/protected properties has worked without it since PHP 8.1. The calls were no-ops emitting deprecation notices on every test run.

Why

During the 0.5.0 release attempt, ApplicationTest::"it calls module boot callbacks after bindings are registered" failed once in a parallel run with Failed asserting that false is true. The test passed in isolation and on the next parallel re-run. Root cause: two parallel workers got the same uniqid() value, used it for the same $baseDir = sys_get_temp_dir() . '/marko-test-' . $uniqueId, and one worker's appTestCleanupDirectory() wiped the other worker's module.php before $app->initialize() could load it — so the boot callback never fired and the asserted $GLOBALS flag stayed false.

This is a release-blocking issue: any future ./bin/release.sh run could hit the same flake and abort. Fixing once removes the class of failure entirely.

The deprecation cleanup is a small companion fix found while investigating — pest --display-deprecations showed the three setAccessible calls firing, and removing them brought the deprecation count from 2 → 0.

Fix Mechanics

uniqid() collisions:
```diff

  • $baseDir = sys_get_temp_dir() . '/marko-test-' . uniqid();
  • $baseDir = sys_get_temp_dir() . '/marko-test-' . bin2hex(random_bytes(8));
    ```

Same call shape, but bin2hex(random_bytes(8)) returns 16 hex chars of cryptographic entropy — effectively zero collision probability vs uniqid()'s microtime-derived 13 chars. Drop-in compatible with all use sites (opaque identifier in temp paths and namespace suffixes).

The single uniqid('test_', true) call in media-gd tests is left alone — it already includes the more_entropy argument.

setAccessible(true) removal: just delete the line. Since PHP 8.1, ReflectionProperty::getValue() on a private/protected property works without prior setAccessible(true).

Test plan

  • Full destructive suite (pest -c phpunit.xml --parallel) run 3 times consecutively post-fix:
    • Run 1: 4898 passed, 0 failed, 0 deprecated, 138 notices, 2 skipped
    • Run 2: 4898 passed, 0 failed, 0 deprecated, 138 notices, 2 skipped
    • Run 3: 4898 passed, 0 failed, 0 deprecated, 138 notices, 2 skipped
  • Pre-fix state: intermittent 1-test failure + persistent 2 deprecation notices
  • No production code touched — packages/*/src/** untouched

🤖 Generated with Claude Code

… deprecated setAccessible() calls

Two related test-suite hygiene fixes:

## uniqid() collisions under parallel pest

PHP's uniqid() is microtime-based and the docs explicitly warn it does
NOT guarantee uniqueness without additional entropy. When pest runs with
--parallel (10 worker processes), two workers can call uniqid() in the
same microsecond, producing identical strings. Tests that use the result
as a temp-directory name then race on filesystem state — e.g. one
worker's appTestCleanupDirectory() can wipe another worker's module.php
before $app->initialize() reads it, leaving a boot callback unfired and
the test failing with a confusing "false is not true" assertion.

This actually triggered a release-blocking failure on
ApplicationTest::"it calls module boot callbacks after bindings are
registered" during the 0.5.0 release run. The test passes in isolation
and re-runs of the parallel suite passed cleanly, confirming the flake
nature.

Mechanical replacement of bare `uniqid()` with `bin2hex(random_bytes(8))`
across all 54 test files that used it — same call shape, but
cryptographically unique 16-hex-char output instead of 13 chars of
microtime-derived entropy. Zero collision risk.

The single `uniqid('test_', true)` call is left alone — it already
adds entropy via the `more_entropy` arg.

## ReflectionProperty::setAccessible() deprecation in PHP 8.5

PHP 8.5 deprecates ReflectionProperty::setAccessible() because, since
PHP 8.1, reflection on private/protected properties just works without
needing to flip accessibility. Three calls in
packages/core/tests/Unit/ApplicationTest.php were emitting deprecation
notices on every run.

Removed all three — they were no-ops since 8.1, and now silent under
8.5.

## Verification

- Full destructive suite (`pest -c phpunit.xml --parallel`) run 3
  times consecutively post-fix: 4898 passed, 0 failed, 0 deprecated,
  0 errors. Previously: intermittent 1-test failure plus persistent
  2 deprecation notices.
@github-actions github-actions Bot added the bug Something isn't working label May 2, 2026
@markshust markshust merged commit cf89129 into develop May 2, 2026
1 check passed
@markshust markshust deleted the feature/fix-test-uniqid-flake branch May 2, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant