Skip to content

Flaky test on phpunit (8.5, trunk): test_on_before_delete_includes_published_comment_tids #52

@kraftbj

Description

@kraftbj

Summary

Atmosphere\Tests\Test_Atmosphere::test_on_before_delete_includes_published_comment_tids fails intermittently on the phpunit (8.5, trunk) matrix only — passes consistently on PHP 8.2 / 8.3 / 8.5 + WP latest, and the most recent push to trunk itself was green. Re-running the failed job typically passes the test on the second attempt without any code change.

Observed across multiple runs of PR 51:

Commit First run Rerun
8f879ff FAIL PASS
cc08013 PASS
3b50792 FAIL PASS

Failure

1) Atmosphere\Tests\Test_Atmosphere::test_on_before_delete_includes_published_comment_tids
Expected atmosphere_delete_records to be scheduled with the published comment TIDs.
Failed asserting that false is not false.

The test sets up a post with META_TID = 'bsky-tid-root', Document::META_TID = 'doc-tid-root', and 3 comments (2 published, 1 orphan), then calls on_before_delete() and asserts that wp_next_scheduled('atmosphere_delete_records', ['bsky-tid-root'], 'doc-tid-root', ['bsky-tid-a', 'bsky-tid-b']) returns non-false. Intermittently wp_next_scheduled returns false on the 8.5/trunk shard.

Likely root cause (not investigated deeply)

PHP 8.5 + WP nightly emits ~hundreds of Method ReflectionMethod::setAccessible() is deprecated since 8.5 warnings during the run (most originating from class-test-reaction-sync.php). The test uses fixed string TIDs, so test ordering / leftover state from other suites could be interfering with how wp_next_scheduled matches cron event args, but I haven't traced exactly which prior test leaks state.

Suggestions

  • Make the test resilient to leftover scheduled events with the same hook (clear atmosphere_delete_records in set_up() / tear_down()).
  • Use unique per-test TIDs (e.g. 'bsky-tid-root-' . wp_generate_password(8, false)) so a stale scheduled event from another test can't shadow the assertion.
  • Independently, fix the ReflectionMethod::setAccessible() deprecation in class-test-reaction-sync.php — the calls have been no-ops since PHP 8.1 and can be removed.

Repro

Run the full suite on the phpunit (8.5, trunk) matrix repeatedly. Observed failure rate ~1 in 3.

Not blocking PR 51

The failing test is unrelated to PR 51's changes (delete-records cron scheduling vs. teaser-thread collapse composition). All other matrices pass; reruns reliably go green. Filing this so the underlying flake gets addressed independently.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions