Skip to content

fix: resolve global state pollution causing random-order test failures in HTTP suite#10372

Open
gr8man wants to merge 8 commits into
codeigniter4:developfrom
gr8man:fix/http-random-order-tests
Open

fix: resolve global state pollution causing random-order test failures in HTTP suite#10372
gr8man wants to merge 8 commits into
codeigniter4:developfrom
gr8man:fix/http-random-order-tests

Conversation

@gr8man

@gr8man gr8man commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Description
This PR resolves multiple test pollution issues causing execution failures when tests in the HTTP system suite are executed in random order (--order-by=random).
Ref: #9968
Key Changes:

  • system/HTTP/IncomingRequest.php: Set the globally retrieved Negotiator service to non-shared (Services::negotiator($this, false)) to prevent cross-request negotiation state bleeding.
  • tests/system/HTTP/UserAgentTest.php, tests/system/HTTP/NegotiateTest.php: Added class-level #[BackupGlobals(true)] and called $this->resetServices() in setUp() to restore mutated superglobals and clear mock services.
  • tests/system/HTTP/RedirectResponseTest.php, tests/system/HTTP/ResponseTest.php, tests/system/HTTP/DownloadResponseTest.php: Added #[BackupGlobals(true)] to prevent superglobals mutation leakage.
  • tests/system/HTTP/ResponseTest.php: Injected a clean, empty superglobals mock inside testSetLink to isolate the URL generation from dirty query string pollution (e.g. code=good).
  • tests/system/HTTP/IncomingRequestTest.php: Corrected unmatched close parenthesis typos in header strings and corrected testNegotiatesLanguage assertion (changed expected output from 'en' to 'da') that was previously green only because of stale negotiator cache pollution.
    Checklist:
  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@gr8man

gr8man commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

IMPORTANT

Note to reviewers and developers: Please ensure that tests are run regularly with the --order-by=random flag during development. Identifying test pollution and global state leakage bugs at a later stage is extremely difficult and time-consuming, as these failures only manifest under very specific test suite execution orders.

@gr8man gr8man force-pushed the fix/http-random-order-tests branch from 1fcd77e to dc8de21 Compare July 3, 2026 21:51

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the PHPUnit "Random Seed" from the failing random-order run. That would make it much easier to reproduce the issue locally and confirm that each change is necessary.

This PR is difficult to review as-is because it mixes test isolation fixes with framework behavior changes. Some changes may be valid, but without a reproducing random seed and a per-change explanation, it is hard to verify whether they are necessary.

Comment thread system/HTTP/IncomingRequest.php Outdated
Comment thread system/HTTP/MessageTrait.php Outdated
Comment thread system/HTTP/MessageTrait.php Outdated
Comment thread system/HTTP/RedirectResponse.php Outdated
@gr8man gr8man force-pushed the fix/http-random-order-tests branch from 857504f to cbb953c Compare July 8, 2026 18:55
@github-actions github-actions Bot added the testing Pull requests that changes tests only label Jul 8, 2026

@paulbalandan paulbalandan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to confirm these changes is to add HTTP in the tested components for random order. Same with your other PR touching random order. The only caveat is that the fix should be fully complete. If not yet, then do not add to the random order suite yet.

@github-actions github-actions Bot removed the testing Pull requests that changes tests only label Jul 8, 2026
@gr8man

gr8man commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

The HTTP test suite has been heavily tested locally. I've executed the HTTP test suite over 500 times using --order-by=random to ensure there are absolutely no lingering global state pollution issues or edge case failures.

All of the reviewer's feedback has been addressed, and test isolation issues across the HTTP module (including IncomingRequestTest, MessageTest, and CorsTest) are now fully resolved.

I've also uncommented the HTTP component in .github/scripts/random-tests-config.txt. The HTTP module is fully ready and stabilized for the random order execution suite!

@gr8man gr8man requested a review from michalsn July 8, 2026 20:02

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes. My only remaining concern is that resetServices() is not consistently placed before parent::setUp(). In general, I would expect resetServices() to run first so the parent setup can rebuild the default test mocks afterward. That said, tests are passing, so I guess we're good.

@gr8man

gr8man commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Good catch! You're absolutely right about the execution order of resetServices(). Even though the tests happened to pass, doing it before parent::setUp() prevents wiping out any default test mocks set up by the framework. I'll fix this inconsistency across the HTTP test suite right away. I have performed over 100 tests locally and everything is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants