Skip to content

Conversation

javiercasares
Copy link
Member

@javiercasares javiercasares commented Mar 16, 2024

This is the Cloudfest Hackathon 2024 PR.

This PR solves:

  • Multi PHP. There is a new env option where you can set all the different PHP versions you want to test.
  • Multi Environment. You can set a label to differentiate, for example, your “shared”, “vps”, or “cloud” setups.
  • Check all commits. Until now, only the latest commit can be checked. With this new option, we can check the latest 10 commits so none of the commits are missing if they are approved without a lot of time between them.

The test-reporter will be upgraded to comply with these changes.

@javiercasares
Copy link
Member Author

- add the label
- test dir is a dir (a tmp one)
- no cert validation by default
- simple WP by default
- support for labels
- read latest commits
@javiercasares
Copy link
Member Author

The code is not very beauty, but it should work...

@javiercasares javiercasares marked this pull request as ready for review March 17, 2024 14:21
@javiercasares javiercasares changed the title Multi PHP Multi PHP + Multi Environment + Check all commits Mar 17, 2024
this is a first step to include some documentation inside the project, and not only in the handbook page.
javiercasares and others added 2 commits September 28, 2024 09:53
Co-authored-by: Matthias Pfefferle <[email protected]>
Co-authored-by: Matthias Pfefferle <[email protected]>
- Descriptions in 80 chars columns
- Some Yoda fixes
- Fix the "tab" vs "space"
- Check some "comments" that should not be there (they were because some errors on the developing era)
- Better comments / PHPDoc
- Improved code quality (do the same, in a better way)
- reduce duplicated code (that's something we didn't care at Cloudfest)
- updated the Runner Compatibity with PHP 7.2 (as WordPress is)
- Improved readability
- default .env updated from source
- other fixes
- better documentation
- details on each value at the configuration file
- scripts so you can run (and update) the software automatically.
@javiercasares
Copy link
Member Author

I applied the suggestion @pfefferle did on WordCamp Europe, and reduce a lot of duplicated code that we couldn't do at the Cloudfest Hackathon, mostly on the parts when testing multiple PHP versions.

I did some improvements on the documentation, with some scripts so the software (from the main branch) is updated and maintained automatically.

I've been doing some tests in my local machine, and on my production server (where I was doing the former tests) and looks like everything is working.

On my side, I think everything is in order, and we can approve this and put it available for everyone.

@benniledl
Copy link
Contributor

benniledl commented Oct 2, 2024

Running tests in parallel?

Running the PHPUnit test suite across 7 different PHP versions is taking a long time in my environment. Is there a way to run the tests in parallel to speed things up?

Preparing a new environment for every version necessary?

Also, is it necessary to create a completely new environment for each PHP version? So far, I've been running the prepare.php file for the first PHP version, running the tests then report.php then using a script to search and replace the PHP executable in the configuration files before rerunning the tests and reporting again. This approach seemed to work.

Copying the environment + Running in parallel

I could see that using the same environment for multiple php versions in parallel could cause problems, is it maybe possible to just copy one environment to a different directory and then replace the php version in the config? Since preparing an environment also takes a long time for me.

@WesRapyd
Copy link

What is the status of this pull ???

@kittenkamala
Copy link
Contributor

I would like to have someone with access to hosting envs test this before we merge if possible

@desrosj
Copy link
Member

desrosj commented Aug 13, 2025

I'm going to put some time towards getting this tested and merged over the course of the next few weeks.

At Bluehost, we currently have a fork that we used to make changes directly and get our tests back up and running. However, it's a bit of a pain to maintain those small differences while keeping the fork in sync with changes here. Some of the changes actually unintentionally accomplish multi-php but just through the GitHub Action.

This PR is quite a bear, though. I am going to try and pull all of the unrelated documentation changes into a separate pull request, and all other unrelated functional changes into another in order to make this more focused.

@kittenkamala
Copy link
Contributor

I'm going to put some time towards getting this tested and merged over the course of the next few weeks.

At Bluehost, we currently have a fork that we used to make changes directly and get our tests back up and running. However, it's a bit of a pain to maintain those small differences while keeping the fork in sync with changes here. Some of the changes actually unintentionally accomplish multi-php but just through the GitHub Action.

This PR is quite a bear, though. I am going to try and pull all of the unrelated documentation changes into a separate pull request, and all other unrelated functional changes into another in order to make this more focused.

Thank you so much, @desrosj !!

desrosj added a commit that referenced this pull request Aug 19, 2025
desrosj added a commit that referenced this pull request Aug 19, 2025
This wraps all string values in double quotes (`"`) to prevent word splitting.

This is being split off from #212 since this specific change was unrelated to accomplishing the goal of that pull request.

This also applies the change for all variables expecting a string type instead of just some.

Co-Authored-By: Javier Casares <[email protected]>
Copy link
Member

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

I've been extracting out some of the changes in this pull request that are unrelated to achieving multi PHP/multi environment/multi commit testing and tracking.

There are some merge conflicts that need to be resolved after (and if) #262, #256, and #255 are merged, but for now I've left some feedback on the parts specific to the goals of the PR.

docs/1-setup.md Outdated
`8.0+/bin/php8.0,8.1+/bin/php8.1`
Use as much versions as you want, but it will take more time. The idea is to put all the versions offered to users.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Use as much versions as you want, but it will take more time. The idea is to put all the versions offered to users.
Use as many versions as you'd like, but keep in mind that it will take more time. Ideally all versions offered to users are tested.

Comment on lines +107 to +108
$WPT_PREPARE_DIR_MULTI = $WPT_PREPARE_DIR . '-' . crc32( $php_multi['version'] );
$WPT_TEST_DIR_MULTI = $WPT_TEST_DIR . '-' . crc32( $php_multi['version'] );
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why the version needs to be obfuscated? $php_multi['version] should only be set to 8.1, 8.0, etc. Including the version in the directory name in plain text could be very helpful for debugging purposes.

What about something like this instead?

Suggested change
$WPT_PREPARE_DIR_MULTI = $WPT_PREPARE_DIR . '-' . crc32( $php_multi['version'] );
$WPT_TEST_DIR_MULTI = $WPT_TEST_DIR . '-' . crc32( $php_multi['version'] );
$WPT_PREPARE_DIR_MULTI = $WPT_PREPARE_DIR . '-' . str_replace( '.', '-', $php_multi['version'] );
$WPT_TEST_DIR_MULTI = $WPT_TEST_DIR . '-' . str_replace( '.', '-', $php_multi['version'] );

It occurs multiple times, so I think a helper function in functions.php may be good.

function directory_name_from_php_version( $version ) {
	return str_replace( '.', '-', $version] );
}

Comment on lines 48 to +54
# (Optionally) define the PHP executable to be called
export WPT_PHP_EXECUTABLE=${WPT_PHP_EXECUTABLE-php}

# (Optionally) array of versions
# like: "8.1=/bin/php8.1;8.2=/bin/php8.2;8.3=/bin/php8.3"
export WPT_PHP_EXECUTABLE_MULTI=""

Copy link
Member

Choose a reason for hiding this comment

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

Is a new environment variable necessary? Could WPT_PHP_EXECUTABLE be made to accept single or array values instead?

The default can still remain as php, and the code below responsible for parsing the value can check for the presence of = and/or ; characters to process as an array.

$WPT_SSH_OPTIONS = trim( getenv( 'WPT_SSH_OPTIONS' ) ) ? : '-o StrictHostKeyChecking=no';
$WPT_TEST_DIR = trim( getenv( 'WPT_TEST_DIR' ) );
$WPT_RM_TEST_DIR_CMD = trim( getenv( 'WPT_RM_TEST_DIR_CMD' ) ) ? : 'rm -r ' . $WPT_TEST_DIR;
$WPT_PREPARE_DIR = trim( getenv( 'WPT_PREPARE_DIR' ) ) ? : '/tmp/wp-test-runner';
Copy link
Member

Choose a reason for hiding this comment

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

Applying these new, default values has been extracted out into #262.

@@ -0,0 +1,5 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

It's not quite clear what each of these represent based on their names. Have the executed_commits been successfully tested and reported? If the runner fails to be configured and run the tests is it included in that list?

Are pending_commits in a queue to be tested? Something else? Are testing_commit being tested right now? Will this never be more than one?

Also, not sure if this file is here for testing purposes, but it should probably be added to the .gitignore file.

$commit_sha = null;
$commits_file = __DIR__ . '/commits.json';

if ( file_exists( $commits_file ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

If the file does not exist and $WPT_COMMITS, the runner should copy the example file to ensure the same hashes are not retested.


# Check all commits
# 0 = latest
# 1 = all
Copy link
Member

Choose a reason for hiding this comment

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

All is a bit misleading here because the API caps the results per page at 100. The default is 30.

I would change this to an on or off value. If it's on, the runner will query for the last 30 commits and add them to the queue if they have not yet been tested. If it's off, then only the most recent commit when the runner starts should be tested.

Regardless of this value, though, I think that the commits.json file should still be used to track what has been tested. This will help avoid re-running the suite for a commit that has already been tested and reported.

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

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Process all commits Update Runner to Support Multiple Report Submissions From Hosts

7 participants