FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Run tests in parallel by default by NickSdot · Pull Request #22939 · php/php-src · GitHub

/ php-src Public

Run tests in parallel by default - #22939

Merged
arnaud-lb merged 10 commits into
php:masterfrom
NickSdot:perf/parallel-by-default
Aug 10, 2026
ConversationCommits (10)ChecksFiles changed
Merged

Run tests in parallel by default#22939
arnaud-lb merged 10 commits into
php:masterfrom
NickSdot:perf/parallel-by-default

Conversation

NickSdot commented Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Extracted from #22917.

Runs the test suite in parallel by default; automatically selects up to 10 workers. Explicit -j1 remains the opt-out to run tests serial. Smaller batches improve balancing.

Intended to merge after #22957 so eligible Windows test subprocesses no longer use the shell wrapper when parallel execution becomes the default.

Result

Values in seconds; measured on macOS.

mode master this PR saved speedup
(none) 2223.224 408.009 1815.215 5.45×

Reproduction:

sapi/cli/php run-tests.php -q

Ref: 9b36b91 (review comments)
cc @arnaud-lb

NickSdot mentioned this pull request Jul 30, 2026
Draft

LamentXU123 commented Jul 30, 2026

Copy link
Copy Markdown
Member

Not an expert in conflicts file. Should cc @bukka in this.

LamentXU123 removed their request for review July 30, 2026 12:36

NickSdot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Not an expert in conflicts file. Should cc @bukka in this.

FWIW, the doc change is only a clarification because after this tests run in parallel by default. This PR has no changes to CONFLICTS itself. 🫡

NickSdot marked this pull request as draft July 30, 2026 13:24
arnaud-lb approved these changes Jul 30, 2026
View reviewed changes

arnaud-lb 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me otherwise, but I will wait for other reviews

Comment thread tests/run-test/automatic_worker_limit.phpt Outdated
Comment thread run-tests.php
arnaud-lb reviewed Jul 30, 2026
View reviewed changes
Comment thread run-tests.php Outdated
NickSdot force-pushed the perf/parallel-by-default branch from 06569f4 to 9fb7077 Compare July 30, 2026 15:09
NickSdot mentioned this pull request Jul 30, 2026
Merged
NickSdot force-pushed the perf/parallel-by-default branch from 0374c74 to 472acd1 Compare July 30, 2026 21:23
NickSdot commented Jul 31, 2026
View reviewed changes
Comment thread run-tests.php Outdated
NickSdot marked this pull request as ready for review July 31, 2026 06:34
arnaud-lb approved these changes Jul 31, 2026
View reviewed changes

arnaud-lb 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me otherwise!

I will wait a few days and merge if there are no objections.

Comment thread run-tests.php Outdated
Comment thread run-tests.php Outdated
Comment thread run-tests.php Outdated
NickSdot mentioned this pull request Jul 31, 2026
Open
NickSdot force-pushed the perf/parallel-by-default branch from 9e8de6b to 5a874af Compare August 3, 2026 20:28

NickSdot commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks good to me otherwise!

I will wait a few days and merge if there are no objections.

Friendly reminder, @arnaud-lb. 🫡

If this is merged #22995 would have a slight change. The same is true for #22946, so if you would merge here it would be nice if you could look at the Windows one-liner along with it. I plan to later today push the update for the CI PR.

Monday is a good day to merge, isn't it? 🤓 Will be around to have an eye on CI in case anything unexpected happens!

Edit: does this need a NEWS or INTERNALS entry?

arnaud-lb commented Aug 10, 2026

Copy link
Copy Markdown
Member

Yes, this is worth a NEWS and INTERNALS entry. Please add them, and I will merge :)

NickSdot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Yes, this is worth a NEWS and INTERNALS entry. Please add them, and I will merge :)

That's done! 🫡

arnaud-lb merged commit 1d2ea5c into php:master Aug 10, 2026
1 check passed

arnaud-lb commented Aug 10, 2026

Copy link
Copy Markdown
Member

Thank you!

NickSdot deleted the perf/parallel-by-default branch August 10, 2026 09:34
andypost mentioned this pull request Aug 12, 2026
Merged
NickSdot mentioned this pull request Aug 12, 2026
Merged
This was referenced Aug 12, 2026
Open
Merged

remicollet commented Aug 14, 2026

Copy link
Copy Markdown
Member

Notice, this also affects extensions (pie, pecl...)

So some relying on default behavior need a fix (ex: yar PR #189, memcached PR #581, and probably others)

NickSdot commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Notice, this also affects extensions (pie, pecl...)

So some relying on default behavior need a fix (ex: yar PR #189, memcached PR #581, and probably others)

@arnaud-lb is this something I should look into or is this acceptable for 8.6 and hence something extensions should take care of (or simply run with -j1)? I'd assume it's fine?

@remicollet what's your take? Like, running with j1 brings back the old behaviour - isn't that fine and extensions can just catch up with flakes (as we do here)? Or do you have something else in mind?

arnaud-lb commented Aug 14, 2026

Copy link
Copy Markdown
Member

As this is documented in UPGRADING.INTERNALS this seems fine to me, but maybe @remicollet has another opinion?

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

arnaud-lb arnaud-lb approved these changes
devnexen Awaiting requested review from devnexen devnexen is a code owner

Assignees

No one assigned

Labels

Category: CI Extension: gd Extension: zip Extension: zlib SAPI: cli

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL