Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH Actions: fix the workflows #135

Merged
merged 9 commits into from Aug 8, 2021

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Aug 7, 2021

I came to send in PR #134 and to turn on testing against PHP 8.1, I stayed to make a lot more fixes to the CI workflows... 馃槀

TL;DR: The tests on PHP 5.3-5.6 were not running at all, which meant that it was missed that the changes in commit 8e0e021 are not compatible with PHP < 7.0.
Turning the tests on, now exposes this.

Please check out the various commit messages and the action run results to get the full picture.

Commit details

GH Actions: ubuntu-16.04 is no longer supported

... use ubuntu-18.04 or ubuntu-latest for 20.04 instead.

Also see:

GH Actions: explicitly set code coverage to none

As no code coverage is being recorded for these builds, it is good practice to explicitly set coverage: none in setup-php.
This fixes a warning during the Composer run on PHP 5.3 stating that Xdebug is on.

GH Actions: enable Composer caching

... by using the ramsey/composer-install action.

This means that the Composer downloads directory for dependencies will be cached and restored on each build. This conserves resources and should also make builds faster.

Ref: https://github.com/marketplace/actions/install-composer-dependencies

GH Actions: split off Psalm to separate workflow

Psalm does not need to be run against multiple PHP versions. Running it once should be enough.

With that in mind, this commit:

  • Introduces a separate, dedicated workflow which only installs and runs Psalm.
  • Removes the Psalm related steps from the CI workflow.
  • Removes Psalm from the tools setting for setup-php.

Note: if so desired I could add Psalm as a separate job to the main workflow instead. Having it as a separate workflow makes sense to my brain ;-)

GH Actions: fix running of the tests

While the builds currently all show "green", if you actually fold out the "PHPUnit Tests" step for the builds in the last CI run against master, you can see that the tests haven't been running on PHP 5.3, 5.4, 5.5, 5.6.

As of PHP 7.0, they have been running, but running against a mismatched PHPUnit version as all test runs are run against PHPUnit 9.5.x, while PHPUnit 9.5 is officially only supported on PHP 7.3 and higher.

Additionally, PHPUnit was being installed 3 (!) times, once via the setup-php action, once via the composer install and once via the php-actions/phpunit action.

To ensure that the tests are always run against the most appropriate PHPUnit version for the PHP version against which the tests are being run, I propose to:

  • Remove the installing of PHPUnit via setup-php.
  • Remove the use of the php-actions/phpunit action.
  • Defer to the Composer installed PHPUnit version in all cases.

I'm also removing the explicit ini settings for the "moderate" and "modern" jobs.
These look like they were copied over from example code, but these values don't have any effect on the test runs in these workflows, so may as well be removed.

For the "low" job, however, having some ini values set prevents the tests erroring out too quickly.

GH Actions: merge "moderate" and "modern" jobs

As there is now effectively no difference anymore between the moderate and modern jobs, these jobs can now be merged into one.

GH Actions: set error reporting to -1

The default setting for error_reporting used by the SetupPHP action is error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT and display_errors is set to Off.

For the purposes of CI, I'd recommend running with -1 and display_errors=On to ensure all PHP notices are shown.

Ref: shivammathur/setup-php#469

GH Actions: enable testing against PHP 8.1

For now, this build is still allowed to fail.

GH Actions: don't fail fast on "low" PHP versions

I'd like to suggest adding this as a temporary measure until the failing code has been fixed.

The fail-fast setting defaults to true and has the effect of cancelling any and all running/waiting builds within the same workflow as soon as one build has failed.

By setting this to false, all builds will be always be run, allowing for easier debugging of the current test failures.

As no code coverage is being recorded for these builds, it is good practice to explicitly set `coverage: none` in `setup-php`.
This fixes a warning on PHP 5.3 stating that Xdebug is on.
... by using the `ramsey/composer-install` action.

This means that the Composer downloads directory for dependencies will be cached and restored on each build. This conserves resources and should also make builds faster.

Ref: https://github.com/marketplace/actions/install-composer-dependencies
Psalm does not need to be run against multiple PHP versions. Running it once should be enough.

With that in mind, this commit:
* Introduces a separate, dedicated workflow which only installs and runs Psalm.
* Removes the Psalm related steps from the `CI` workflow.
* Removes Psalm from the `tools` setting for `setup-php`.
While the builds currently all show "green", if you actually fold out the "PHPUnit Tests" step, you can see that the tests haven't actually been running on PHP 5.3, 5.4, 5.5, 5.6.

As of PHP 7.0, they have been running, but running against a mismatched PHPUnit version as all test runs are run against PHPUnit 9.5.x, while PHPUnit 9.5 is officially only supported on PHP 7.3 and higher.

Additionally, PHPUnit was being installed 3 (!) times, once via the `setup-php` action, once via the `composer install` and once via the `php-actions/phpunit` action.

To ensure that the tests are always run against the most appropriate PHPUnit version for the PHP version against which the tests are being run, I propose to:
* Remove the installing of PHPUnit via `setup-php`.
* Remove the use of the `php-actions/phpunit` action.
* Defer to the Composer installed PHPUnit version in all cases.

I'm also removing the explicit ini settings for the "moderate" and "modern" jobs.
These look like they were copied over from example code, but these values don't have any effect on the test runs in these workflows, so may as well be removed.

For the "low" job, however, having some ini values set prevents the tests erroring out too quickly.
As there is now effectively no difference anymore between the `moderate` and `modern` jobs, these jobs can now be merged into one.
The default setting for `error_reporting` used by the SetupPHP action is `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT` and `display_errors` is set to `Off`.

For the purposes of CI, I'd recommend running with `E_ALL` and `display_errors=On` to ensure **all** PHP notices are shown.

Ref: shivammathur/setup-php#469
For now, this build is still allowed to fail.
I'd like to suggest adding this as a temporary measure until the failing code has been fixed.

The `fail-fast` setting defaults to `true` and has the effect of cancelling any and all running/waiting builds within the same workflow as soon as one build has failed.

By setting this to `false`, all builds will be always be run, allowing for easier debugging of the current test failures.
@paragonie-security paragonie-security merged commit 4ca89f2 into paragonie:master Aug 8, 2021
@jrfnl jrfnl deleted the feature/ghactions-fix-it branch August 8, 2021 11:00
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.

None yet

2 participants