Run PHPUnit jobs across multiple PHP versions #46510
Conversation
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
anton-vlasenko
changed the title
… document It can be helpful to track a location in an HTML document while updates are being made to it such that we can instruct the Tag Processor to seek to the location of one of the bookmarks. In this patch we're introducing a bookmarks system to do just that. Bookmarks are referenced by name and handled internally by a tracking object which will follow all updates made to the document. It will be possible to rewind or jump around a document by setting a bookmark and then calling `seek( $bookmark_name )` to move there. Co-authored-by: Adam Zielinski <adam@adamziel.com> Co-authored-by: Dennis Snell <dennis.snell@automattic.com>
dmsnell
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
from
78d42bb
to
fd9a0a2
Compare
anton-vlasenko
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
from
fd9a0a2
to
7fe4a56
Compare
anton-vlasenko
changed the title
anton-vlasenko
changed the title
| @@ -17,6 +17,9 @@ yarn.lock | |||
| /composer.lock | |||
|
|
|||
| .cache | |||
| !/.cache/ | |||
| /.cache/** | |||
| !/.cache/.gitkeep | |||
There was a problem hiding this comment.
This ignores all .cache/.gitkeep files but /.cache/.gitkeep.
The /.cache/ folder is needed for the PHP Coding Standards CI job.
There was a problem hiding this comment.
This deserves an explanatory comment inline as it's going to be really hard to find this PR comment when someone is trying to understand why the .cache directory is so zig-zagged here. Can we add the explanation, and include why a simpler formulation of the rules cannot provide what we need?
Is it not possible to have PHP Coding Standards use another directory? What is the reason for its use of .cache here anyway?
There was a problem hiding this comment.
I also now see that we're telling git to keep the .cache directory, which seems surprising to me. why do we need to do this?
if this weren't .gitkeep'd then this surprising change to .gitignore presumably would also not be necessary
There was a problem hiding this comment.
The reason for zig-zagging is the following:
It is not possible to re-include a file if a parent directory of that file is excluded.
This deserves an explanatory comment inline as it's going to be really hard to find this PR comment when someone is trying to understand why the .cache directory is so zig-zagged here.
I agree. Fixed in 89a4d738e13c7d6ff51d54cfa2477eb0c713c67b
Currently, Gutenberg ignores all .cache folders (regardless of their location).
For compatibility reasons, these folders should still be ignored, while allowing the /.cache folder (in the root).
I haven't been able to come up with a simpler pattern that would allow that.
if this weren't .gitkeep'd then this surprising change to .gitignore presumably would also not be necessary
Git doesn't allow to commit empty folders. If it did, I would commit an empty folder.
Is it not possible to have PHP Coding Standards use another directory? What is the reason for its use of .cache here anyway?
Yes, it's possible, but I don't understand what's wrong with /.cache.
A similar Core job uses a folder with the same name - .cache.
There was a problem hiding this comment.
sure but why do we need or want to commit an empty directory?
There was a problem hiding this comment.
Because PHPCS expects the directory to which to write the cache file to exist. It will not create it.
So if the ruleset dictates that the cache file should be written to the .cache directory, that directory must exist as otherwise PHPCS will error out.
You could, of course, choose to just let the cache file be written to the system temp directory or to the project root...
Note: the PHPCS cache file itself should definitely be gitignored.
There was a problem hiding this comment.
my suspicion was that it was something like this, but if that's the case, that doesn't explain why it needs to be in git, it merely notes that the directory needs to be present when running the linter.
if the entire purpose of coercing git to include an empty directory in the repo is so that a linter can run in CI then it would be simpler to create the directory when running the CI script with mkdir -p .cache or the equivalent.
There was a problem hiding this comment.
Well, that depends. If the ruleset contains the cache directive, local CS runs will also expect the directory.
If the caching is only used in CI, I agree, a mkdir would suffice, but then again, why would the caching be limited to CI ?
Also note - in most projects which use a .cache directory for the PHPCS cache placement, it is generally because they want to cache those files in CI and Travis (yes, that's the completely outdated reason), didn't allow for caching individual files, only allowed for caching directories, which is why people resorted to having a .cache directory for this.
GH Actions, however, does allow for caching individual files, so I honestly don't know why that .cache directory is still used, especially for projects setting this up from scratch. (I can understand people not having revisited their previous reason for setting it up when converting from Travis to GHA, but that's not the case here)
| # http://man7.org/linux/man-pages/man1/date.1.html | ||
| - name: "Get last Monday's date" | ||
| id: get-date | ||
| run: echo "date=$(/bin/date -u --date='last Mon' "+%F")" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Can we get an explanation of why we want to clear the cache once a week?
Generally time-based caches are useful in rare and somewhat emergency situations.
What is the reason we can't use a content-based cache?
Also do we know we need the cache? What do we expect it to provide for us?
There was a problem hiding this comment.
This cache is both content and time-based.
hashFiles('**/composer.json', 'phpcs.xml.dist')
ensures that the cache key is content-based.
Also do we know we need the cache? What do we expect it to provide for us?
The main benefit of using the cache is to speed up the CI jobs.
There was a problem hiding this comment.
What have you measured as the speedup in the CI jobs system?
There was a problem hiding this comment.
In my tests,
phpcs -n --report-full
takes about 12 seconds when the cache is disabled and 0 seconds (that's what GitHub shows) when the cache is enabled.
As the quantity of .php files in the project is growing, this difference will only increase over time.
There was a problem hiding this comment.
hm. what if we introduced this first without the cache? in the grander scheme of all the test suites 12s is a drop in the bucket but the cache is going to likely create a non-trivial amount of headache at the point it causes problems in a PR.
I don't think these tests are going to be the slowest workflow suite, which means they aren't in the hot path, and if we merge first without the cache then if we think one is truly warranted then we can make a PR and gather a statistical measurement to determine if, when put into the system with 1700 open branches and lots of CI activity, if it still makes 12s of an improvement and if we think that's worth the additional complexity
There was a problem hiding this comment.
Can we get an explanation of why we want to clear the cache once a week?
What is the reason we can't use a content-based cache?
Without having looked at the implementation in this particular workflow...
- Caches in GitHub Actions work differently than people often expect from other CI systems.
- A cache in GHA is created based on a key and NEVER updated.
- The only way to vacate the cache is to create a new cache with a new key.
So, for caches where the key doesn't change often, the cache tends to go more and more out of date over time. Adding a time based factor to the key helps to vacate the cache regularly.
Also see: ramsey/composer-install#234
hm. what if we introduced this first without the cache?
In case it would be decided to go that way, I would strongly recommend adding the --no-cache CLI argument to the PHPCS command in GHA.
As the cache directive is in the ruleset, PHPCS would otherwise needlessly create the cache file from scratch each time, which has a significant impact on the runtime of PHPCS due to the file-write operations.
As CLI args overrule the ruleset, adding no-cache should already speed up the CS run.
.github/workflows/phpunit-tests.yml
Outdated
| # It must be removed before running the tests. | ||
| - name: Remove incompatible Composer packages | ||
| if: ${{ matrix.php < '7.2' }} | ||
| run: composer remove spatie/phpunit-watcher --dev --no-update |
There was a problem hiding this comment.
if this breaks in supported PHP versions, why not remove the dependency altogether? I'm worried about the disconnect between requiring this package and having this line in the workflow YML. best case scenario is that when this dependency is no longer required, we'll still be running this step, which is benign, but also is going to confuse someone why it's there.
is there a way to have composer tell us which packages are not compatible? or should we file a bug report with spatie/phpunit-watch to have that project list a minimum version if one is missing?
There was a problem hiding this comment.
if this breaks in supported PHP versions, why not remove the dependency altogether?
This is because spatie/phpunit-watcher is required by the npm run test:php:watch command.
Thus it cannot be simply removed.
I was going to create a new GB issue to discuss the possibility of replacing it with a similar package that supports PHP < 7.2 (if that substitute package exists).
is there a way to have composer tell us which packages are not compatible?
Yes, composer already does that. It knows that this package is not compatible with PHP < 7.2 and refuses to install it if the current PHP version is lower than 7.2. So it should be removed before installing other packages (otherwise composer install fails).
or should we file a bug report with spatie/phpunit-watch to have that project list a minimum version if one is missing?
No, it's not missing. https://github.com/spatie/phpunit-watcher/blob/master/composer.json#L19
There was a problem hiding this comment.
Yes, composer already does that. It knows that this package is not compatible with PHP < 7.2 and refuses to install it if the current PHP version is lower than 7.2. So it should be removed before installing other packages (otherwise composer install fails).
What I meant was that maybe we can avoid hard-coding a specific dependency as we've done here. It looks like if we spit out the tree format of composer show we can get that information, and then process it with a small script.
<?php
require 'vendor/autoload.php';
use Composer\Semver\Semver;
$PHP_VERSION = empty( getenv( 'PHP_VERSION' ) ) ? phpversion() : getenv( 'PHP_VERSION' );
$tree = json_decode( file_get_contents( 'php://stdin' ) );
function works_with_php_version( $version, $package ) {
foreach ( $package->requires as $dep ) {
if ( 'php' === $dep->name && ! Semver::satisfies( $version, $dep->version ) ) {
//echo "Bailing on {$package->name} because of {$dep->version}" . PHP_EOL;
return false;
}
if ( ! empty( $dep->requires ) && false === works_with_php_version( $version, $dep ) ) {
return false;
}
}
return true;
}
$unsupported = array();
foreach ( $tree->installed as $package ) {
if ( ! works_with_php_version( $PHP_VERSION, $package ) ) {
$unsupported[] = $package->name;
}
}
echo implode( ',', $unsupported );Invoked as composer show --tree --format=json | php php-supported.php this will spit out a list of packages that have dependencies which aren't supported on the running PHP version, or the version set in PHP_VERSION if such an ENV is set.
But it also includes another package not listed, and to the best of my ability to double-check it, it's right according to the listed minimum versions (when I set PHP_VERSION=5.6 and run it).
spatie/phpunit-watcher,yoast/phpunit-polyfills
While maybe someone registered the wrong minimum version, or we simply haven't hit the part that breaks yet, this is why I think it'd be good at least to explore determine this through inspecting the package definitions. The list will undoubtedly change more rapidly than we come around to update it.
Edit: I should have noted that Semver is also already pulled in to our composer deps.
There was a problem hiding this comment.
Thanks for sharing the script.
we can avoid hard-coding a specific dependency as we've done here
Out of all installed Composer packages, this is the only package that gives trouble.
I didn't see the need to write a script for detecting such packages when I was working on the CI job, as this was the only "problematic" package.
If someone adds a composer package that is not compatible with PHP 5.6 or another Core-supported PHP version, I'd prefer that the author of that PR would be able to see the error (i.e., composer install should fail) instead of removing or hiding such packages with the help of a script.
Gutenberg should support the same PHP versions as Core.
If we had proper CI jobs before, spatie/phpunit-watcher wouldn't be merged to the repo (that's my assumption).
Removing spatie/phpunit-watcher before running PHPUnit tests is a workaround until that package is removed or replaced with another package that supports PHP 5.6.
yoast/phpunit-polyfills - I'm not sure I understand why it isn't compatible with PHP 5.6.
Please see this Packagist page:
https://packagist.org/packages/yoast/phpunit-polyfills
There was a problem hiding this comment.
why it isn't compatible with PHP 5.6.
the dependencies of its dependencies state a minimum required version above 5.6. like I wrote above though, we may simply have not run into the places where it will break, or the package authors may have incorrectly listed their requirements.
if you run composer show --tree you will see where it comes in. ironically it's through phpunit so maybe we could safely eliminate that dependency chain as a given since we're already running.
If someone adds a composer package that is not compatible with PHP 5.6 or another Core-supported PHP version, I'd prefer that the author of that PR would be able to see the error (i.e., composer install should fail) instead of removing or hiding such packages with the help of a script.
I totally agree! that's why I asked about removing the package. do we know if anyone would even miss the npm run test:php:watch scripts? I wouldn't, because I don't use that due to the way it obscures the underlying test runner and won't let me do what I want to do with phpunit (and it doesn't even run when I try, immediately fails with an error).
it's not the worst thing to have this hard-coded exception in the code, but I'd love to do all we can to avoid it if possible. it makes me worry that this code will end up permanent in there and we'll lose our memory of why, and then when something is broken or needing maintenance it will make that work difficult, particularly if we end up removing the referenced package.
alternatively, the worst scenario I see which seems to be quite common is that we have all this special-casing hidden deep in our layers of build scripts and then something breaks and there's no linkage between the failure and the code in the build and testing scripts that leads someone to find the problem.
anyway that's more words than it warrants, but maybe we just ask if we can get rid of npm run test:php:watch and not have to deal with this.
There was a problem hiding this comment.
@jrfnl thanks. I'm sure I'm wrong; I must be wrong. but I like being shown how and why I'm wrong, so if you think the script is wrong, well it's right there and you can point it out.
I'm clearly not an expert on this, but I am looking at the output of composer show --tree and for yoast/phpunit-polyfills it demonstrably lists dependencies for phpunit/phpunit which don't list 5.6 support.
as I noted above, it might make sense to special-case phpunit and omit checking its dependencies, given that we are already assuming the presence of phpunit, but what is wrong in the logic in the script?
I'm not just talking about the dependencies we currently have, but I'm wanting to make sure that when those dependencies change that we don't have to remember that this code exists, that it exists here, and remember to update it properly - not a good track record with relying that much on manual process.
yoast/phpunit-polyfills 1.0.4 Set of polyfills for changed PHPUnit functionality to allow for creating PHPUnit cross-version compatible tests
├──php >=5.4
└──phpunit/phpunit ^4.8.36 || ^5.7.21 || ^6.0 || ^7.0 || ^8.0 || ^9.0
├──doctrine/instantiator ^1.3.1
│ └──php ^7.1 || ^8.0
├──ext-dom *
├──ext-json *
├──ext-libxml *
├──ext-mbstring *
│ └──php >=7.1
├──ext-xml *
├──ext-xmlwriter *
├──myclabs/deep-copy ^1.10.1
│ └──php ^7.1 || ^8.0
here's the truncated part of composer show --tree showing some of the dependencies not listing PHP 5.6 support. Maybe I'm overlooking that something is optional, or that there's a way to know if these will be supported. I'm definitely relying on package maintainers to list the correct support on their packages.
There was a problem hiding this comment.
I'm sure I'm wrong; I must be wrong. but I like being shown how and why I'm wrong, so if you think the script is wrong, well it's right there and you can point it out.
@dmsnell Sorry, I hadn't looked at the script before - I just responded as the author of the PHPUnit Polyfills.
Looking at the script now, IMO, it is pretty clear where the error is.
- The script will only work after a
composer installhas been run (as it needsvendor/autoload). - It will then analyse based on the dependencies which have been installed, which is highly dependent on which PHP version was used to run
composer install(as there is nocomposer.lockfile exactly for that reason: to allow for installing the correct PHPUnit version for each PHP version).
So... say I run composer install on PHP 8.0, it will install PHPUnit 9.x with the associated dependencies. Neither PHPUnit 9.x, nor any of its dependencies will be compatible with PHP 5.6.
No surprise then that the Polyfills will be flagged as incompatible, but that's not a problem with the polyfills, but a problem with a logic fallacy on which the script is based....
There was a problem hiding this comment.
Sorry, I hadn't looked at the script before - I just responded as the author of the PHPUnit Polyfills.
I definitely wish I were smart enough to not have to read the questions I respond to 🙃
No surprise then that the Polyfills will be flagged as incompatible, but that's not a problem with the polyfills, but a problem with a logic fallacy on which the script is based....
If I understand what you're saying though it seems to imply the script is correct, the logic in the script is correct, and I simply made the mistake of running it after installing packages with a newer version of PHP, something which wouldn't happen in the CI run.
So I think in a way by trying to show that the script is flawed you actually confirmed it's the right logic, because my goal is to determine programmatically what needs to be removed based on the running script in a way that lets us avoid depending on people manually doing all the right steps.
However…
highly dependent on which PHP version was used to run composer install…So... say I run composer install on PHP 8.0, it will install PHPUnit 9.x with the associated dependencies. Neither PHPUnit 9.x, nor any of its dependencies will be compatible with PHP 5.6.
When I tried doing this I actually got the same result with PHP 5.6 where the composer show --tree output still declares yoast/phpunit-polyfills as unsupported, with identical output as listed above.
What I did was:
- clone the Gutenberg repo in a
docker run php:5.6environment - install Composer through their script
- run
rm composer.lock && rm -rf vendor && git reset --hardto make sure that nothing invendorwas stale - run
./composer.phar install - run
./composer.phar show --list
~$ php --version
PHP 5.6.40 (cli) (built: Jan 23 2019 00:04:26)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
~$ ./composer.phar --version
Composer version 2.2.18 2022-08-20 11:33:38In this case though my script failed to run because of something in symphony/polyfill that depended on newer PHP versions. After manually removing spatie/phpunit-watcher it progressed further, but still failed to execute the autoloader due to an issue with unsupported strict_types inside the phpunit's Framework/Assets/Functions.php module.
Maybe my version of Composer is too new? To try and see if this could be the case I tried against with Composer 1.6 from the beginning of 2018 but its output is still the same, still showing phpunit/phpunit pulling in dependencies that rely on PHP 7.1 and newer.
So I guess the current state is that if we want to do this programmatically the script is telling us the information from the installed packages as it should, but we need to modify it to read the Semver dependencies directly instead of through the autoloader, since that fails on PHP 5.6, and also ensure that we're running it on the right set of installed packages.
require 'phar://composer.phar/vendor/composer/semver/src/Constraint/ConstraintInterface.php';
require 'phar://composer.phar/vendor/composer/semver/src/Constraint/MultiConstraint.php';
require 'phar://composer.phar/vendor/composer/semver/src/Constraint/Constraint.php';
require 'phar://composer.phar/vendor/composer/semver/src/Semver.php';
require 'phar://composer.phar/vendor/composer/semver/src/VersionParser.php';
use Composer\Semver\Semver;
$PHP_VERSION = empty( getenv( 'PHP_VERSION' ) ) ? phpversion() : getenv( 'PHP_VERSION' );
$tree = json_decode( file_get_contents( 'php://stdin' ) );
function works_with_php_version( $version, $package ) {
foreach ( $package->requires as $dep ) {
if ( 'php' === $dep->name && ! Semver::satisfies( $version, $dep->version ) ) {
return false;
}
if ( ! empty( $dep->requires ) && false === works_with_php_version( $version, $dep ) ) {
return false;
}
}
return true;
}
$unsupported = array();
foreach ( $tree->installed as $package ) {
if ( ! works_with_php_version( $PHP_VERSION, $package ) ) {
$unsupported[] = $package->name;
}
}
echo implode( ',', $unsupported );I'd still like to better understand where I'm wrong here because I think I have tried doing what you told me I need to do but the results are the same as when I did it before.
There was a problem hiding this comment.
If I understand what you're saying though it seems to imply the script is correct, the logic in the script is correct, and I simply made the mistake of running it after installing packages with a newer version of PHP, something which wouldn't happen in the CI run.
No, as, as you yourself just proved above.
- Your script is intended to find out which dependencies are incompatible so they can be removed before an install is run.
- Your script requires a Composer install to have been run already, which would fail when there are incompatible dependencies.
Maybe my version of Composer is too new?
For an install on PHP 5.6, you can use the latest of the Composer LTS 2.2.x series.
Composer 2.3.0 and higher require PHP 7.2 (or higher).
In this case though my script failed to run because of something in symphony/polyfill that depended on newer PHP versions. After manually removing spatie/phpunit-watcher it progressed further, but still failed to execute the autoloader due to an issue with unsupported strict_types inside the phpunit's Framework/Assets/Functions.php module.
I haven't dug in any deeper now, but I suspect this may have something to do with the default PHP version on your system not being PHP 5.6 and something somewhere along the way starting a new PHP process and using the system default PHP version instead of the PHP version you want it to use. Could that be it ?
So I guess the current state is that if we want to do this programmatically the script is telling us the information from the installed packages as it should, but we need to modify it to read the Semver dependencies directly instead of through the autoloader, since that fails on PHP 5.6, and also ensure that we're running it on the right set of installed packages.
Possibly, and while I appreciate you are enjoying the intellectual exercise of trying to understand this. @anton-vlasenko 's two liner in the GHA script fixes this with a lot less effort and higher accuracy.
There was a problem hiding this comment.
Your script is intended to find out which dependencies are incompatible so they can be removed before an install is run.
that's not the case, but that's how I interpreted your statement when you said "which is highly dependent on which PHP version was used to run composer install". if the running PHP version determines what packages composer will install then I took that to imply it wouldn't install packages that are listed as incompatible.
For an install on PHP 5.6, you can use the latest of the Composer LTS 2.2.x series.
thanks. as indicated in my previous comment I did attempt this on 2.2 and 1.6 versions.
system default PHP version instead of the PHP version you want it to use. Could that be it ?
No I don't think so, because as indicated in my previous comment, PHP 5.6 was the only version of PHP installed within the Docker container. It was both the system default and the only available version to run.
.github/workflows/phpunit-tests.yml
Outdated
| git --version | ||
| svn --version | ||
| composer --version | ||
| locale -a |
There was a problem hiding this comment.
Is all this necessary or mostly useful while developing this branch? Same question for the Docker debug info step below.
There was a problem hiding this comment.
It's not necessary, but it's worthwhile.
E.g., it helps to confirm which version of Composer and/or PHP are used when installing Composer packages and running PHPUnit.
There was a problem hiding this comment.
I'm asking because we don't currently do this for our other workflows, and it's a fair amount of noise, and some duplication (e.g. PHP and composer versions are already available through the matrix and through the PHP install step).
It's not problematic, per se, just curious why it's here as it seems like a step (to remove this debug logging) that might have been overlooked before prepping for final merge into trunk.
There was a problem hiding this comment.
because we don't currently do this for our other workflows
We actually do this.
PHP and composer versions are already available through the matrix and through the PHP install step
Yes, they are through the matrix, but we cannot be sure that the CI jobs use these specific versions (until we log them).
Also, in the matrix, we only specify major and minor PHP versions (the same way it's implemented in Core).
The logging allows us to know the exact PHP version (including the point release) used when running particular CI jobs.
There was a problem hiding this comment.
to know the exact PHP version (including the point release) used when running particular CI jobs.
That information is already available via the setup-php step.
Jonathan and me actually discussed removing a lot of this kind of debug noise from the WP Core workflows recently as the information is already available within the workflow.
There was a problem hiding this comment.
That information is already available via the setup-php step.
I've removed logging Composer and PHP versions.
cb3ee089ee66b68af28305833e5868f7d517454d
2dfb8452e972553650ac309e497cb0fee252be15
anton-vlasenko
changed the title
anton-vlasenko
changed the title
anton-vlasenko
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
from
f11e08d
to
1620c6f
Compare
anton-vlasenko
changed the title
There was a problem hiding this comment.
Thanks for the ping @anton-vlasenko and for setting this up! Great step forward.
I've left a number of comments inline. Take from it what you will.
Other than that, I truly don't understand why NPM and Docker are needed. Seems like a lot of overkill and a roundabout way to do things, which can just be done using a PHP based workflow. Then again, I'm not intimately familiar with GB, so there may be good reasons for it, in which case, it might be a good idea to document those.
.github/workflows/phpunit-tests.yml
Outdated
| php: | ||
| [ | ||
| '5.6', | ||
| '7.0', | ||
| '7.1', | ||
| '7.2', | ||
| '7.3', | ||
| '7.4', | ||
| '8.0', | ||
| '8.1', | ||
| '8.2', | ||
| ] |
There was a problem hiding this comment.
Either specify it like so:
php: ['5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2']or like so:
php:
- '5.6'
- '7.0'
- '7.1'
- '7.2'
- '7.3'
- '7.4'
- '8.0'
- '8.1'
- '8.2' This is Yaml, not PHP.
There was a problem hiding this comment.
I agree with @jrfnl on this, but…
This is Yaml, not PHP.
I don't see the reason to add snark in this comment. The code as written parses as expected and is valid YAML. We don't have to claim that @anton-vlasenko was unaware of which language is being written or put anyone down when their style is different than your style or even when we find stylistic differences in a project and a code submission.
There was a problem hiding this comment.
Sorry if you interpreted my comment as snarky, definitely wasn't meant as such. It was just meant to convey that I understand where the inclination to write it like that comes from, nothing more.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Thanks for the review, @jrfnl.
I see your point. My reasoning for using Docker and
I have nothing against running the unit tests native. I just see more benefits in enabling testing on all PHP versions sooner than later to prevent merging incompatible/sub-optimal PHP code to the repo. |
anton-vlasenko
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
from
22b878c
to
0b7b208
Compare
I fully agree with getting this merged sooner rather than later. This can always be iterated on at a future point in time. 👍🏻 Re: Docker - I have the same objections to WP Core running the tests via Docker.
There are currently tests in the WP Core test suite which are never run in CI due to the "ideal" Docker environment setup. This is not good. Let's be realistic here: we are not building software which will only run in an environment over which we have full control, like in-company software. Note: For Core this is an ongoing discussion and something I would very much like to change (definitely outside the scope of this PR though). For GB, as this is a new test workflow, you have the opportunity to do better than Core, but I fully respect it if you choose to start with this and iterate on it once this is changed in WP Core. |
|
Thanks for listing all the issues with the current testing environment.
I cannot tell for sure that I will backport changes from Core to Gutenberg once the Core testing environment gets changed. I'd appreciate it if you could approve this PR, @jrfnl. |
Props to @jrfnl for finding this bug.
Props to @jrfnl for identifying an issue with PHP 7.4 and 7.4. # Conflicts: # packages/env/CHANGELOG.md
2. Don't put the OS version in the matrix.
…"Set up PHP" step.
2. Move the cache option to the phpcs.xml.dist
…hould be removed as well. Props to @jrfnl.
…ious)." This reverts commit eaef3343f85faa8f50f7465eb70105cac961d0ca.
anton-vlasenko
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
2 times, most recently
from
5d58954
to
4554441
Compare
@jrfnl Yes, there is a Dependabot config in the repo. |
This is needed because: 1. unit-php job has to be preserved as it's set as required in the GH settings. 2. unit-php job now depends on the test-php job.
anton-vlasenko
force-pushed
the
fix/parity-with-cores-php-ci-jobs
branch
from
2ce991c
to
2a371d8
Compare
juanmaguitar
added
the
[Type] Project Management
What?
This PR:
Unit TestsGithub CI job;PHPUnit TestsandPHP Coding Standards(the same way it's implemented in Core);wp-envas it used non-optimal PHPUnit versions on PHP 7.3 and 7.4 (props to @jrfnl);Why?
Gutenberg should support the same PHP versions as Core.
How?
This PR adds 2 new Github CI Jobs:
PHPUnit TestsandPHP Coding Standards.These PHP CI jobs were backported from Core to ensure that we test Gutenberg PRs in the same fashion.
Testing Instructions
PHP_VERSION (multisite) on ubuntu-latestjobs for each supported PHP version (5.6 through 8.2).Props @jrfnl , @dmsnell , @anton-vlasenko