GH Actions: improve workflows

[jrfnl]

Related to #44, follow-up PR to #46

Commit Details

GH Actions workflows: fix PHP set up step

To disable code coverage, the coverage key should be set to none. false is not a valid (or supported) value, so the behaviour may be unpredictable.

Ref: https://github.com/shivammathur/setup-php#disable-coverage

GH Actions: run the tests against all supported PHP versions

The original Travis script ran the following commands against all supported PHP versions, but didn't test the phar:

  - composer test
  - ./parallel-lint --exclude vendor --exclude tests/examples --no-colors .
  - ./parallel-lint --exclude vendor --exclude tests/examples .

The current test job only ran the unit tests against PHP 5.4. It did not run the integration test (running Parallel Lint against its own code) via a direct call to the script anymore at all.
The integration test was basically now only being run for the phar and only in one flavour, though it did do that on all supported PHP versions.

This commit:

• Removes the job which only ran the unit tests against PHP 5.4.
• Adds the running of the above three commands (unit tests and two versions of integrations tests runs via a direct call to the script), to the job which also runs the integration tests via the phar file.
• Updates the command line parameters used for the phar run to be the same as those used for the direct script call integration tests.

GH Actions: actually get the tests running on all PHP versions

This fixes the problem originally identified by @roelofr with the Fatal error: Interface 'JsonSerializable' not found error.

The problem had nothing to do with the PHP setup, but everything with some inane setting of the Nette testing framework as can be seen in the transcript of older, failing trial runs done by @roelofr while setting up the GH Actions workflows:

Run composer test
> @php vendor/bin/tester -p php tests
 _____ ___  ___ _____ ___  ___
|_   _/ __)( __/_   _/ __)| _ )
  |_| \___ /___) |_| \___ |_|_\  v2.3.5

Note: No php.ini is used.

The key here is the Note: No php.ini is used..

As the system php.ini was not being used, no extensions were loaded, which was causing the errors.

This is a change which was introduced in Nette Tester 2.0.0. As of that version, you need to always tell the Nette testing framework explicitly which php.ini file to use, -C tells it to use the system-wide php.ini, with -c you can specify a path to a php.ini file and by default no php.ini is used.
(honestly, why did anyone ever think making that the default was a good idea ?!!?)

As the tests will be running on different versions of the Nette Framework, Nette 1.x for PHP 5.4 and 5.5 and Nette 2.x for PHP 5.6+ and Nette 1.x, does not yet support the -C option and breaks when it is used, I've added a second test script to the composer.json file with the correct command line options to run Nette on PHP 5.4/5.5 and added conditions to the GH Actions workflow to use the correct script depending on the PHP/Nette test framework version being used.

Refs:

• https://tester.nette.org/en/running-tests#toc-c-path
• https://tester.nette.org/en/guide#toc-supported-php-versions
• https://github.com/nette/tester/releases/tag/v2.0.0

GH Actions: allow for manually triggering a workflow

Triggering a workflow for a branch manually is not supported by default in GH Actions, but has to be explicitly allowed.

This is useful if, for instance, an external action script or composer dependency has broken.
Once a fix is available, failing builds for open PRs can be retriggered manually instead of having to be re-pushed to retrigger the workflow.

Ref: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/

GH Actions: report CS violations in the PR

The cs2pr tool will allow to display the results from an action run in checkstyle format in-line in the PR code view, which should improve usability of the workflow results.

This implements this for the style check command.

Includes switching the PHP version to PHP 7.4, as PHP 8.0 is not fully supported yet in PHP_CodeSniffer (full support expected in PHPCS 3.6.0).

Ref: https://github.com/staabm/annotate-pull-request-from-checkstyle

GH Actions: report linting violations in the PR

The cs2pr tool will allow to display the results from an action run in checkstyle format in-line in the PR code view, which should improve usability of the workflow results.

This implements this for the results of running Parallel Lint over its own code.

This also implicitly acts as an integration test for the checkstyle output and the cs2pr integration.

Ref: https://github.com/staabm/annotate-pull-request-from-checkstyle

GH Actions: clean up

• Remove the now redundant Travis script.
• Remove the reference to the Travis script from .gitattributes and add the new .github folder.
• Update the "Build status" badge in the README to show the result of the GH Actions run instead of Travis.

Supersedes and closes #54

---

GH Actions: allow for testing on PHP 5.3 🆕

In anticipation of PR #51 being merged, I'm already adding an extra step to the test job.

PHP_CodeSniffer has a minimum PHP requirement of PHP 5.4, which would block the composer install on PHP 5.3.

By removing that dev dependency ahead of the composer install, the test workflow can also run on PHP 5.3.

GH Actions: fix phar creation 🆕

The phar file should only contain the files of PHP Parallel Lint and any non-dev requirements. It should not include the dev requirements of this package.

As things were, it did.

Fixed now, by doing the composer install with the --no-dev option, both for the Test workflow as well as for the Release workflow.

GH Actions: switch execution order of unit vs integration tests 🆕

... and add continue-on-error to the first of the integration tests.

If/when any of the tests fail, execution of the Test job will stop.

Now, if there is a parse error in the code of any of the Parallel Lint files, with the test execution order as it was, that means the job would already fail on the running of the unit tests and stop there.

However to identify the parse error, the integration tests are more useful, so with that in mind, those will now be run first.

Secondly, if there is a parse error, the first integration test would fail and the second (with colours) would never get executed, while especially in the case of a parse error in the Parallel Lint code itself, it is useful to see the output of both these integration tests.

So, with that in mind, I've set the first of the two to continue-on-error.
As the second integration test would fail anyway, this will never negatively impact the workflow success/failure marking in the end, while it does allow us to see the output of both integration test steps.

[roelofr]

Wow, thanks for finding that Nette thing, I spent far too much time missing that one lousy line.

I had one suggestion to always report linting status to PRs, but also print it in the actions pipeline (so we can see if the --colors works too).

[jrfnl]

@glensc @roelofr Thank you both for the reviews. You are right. (it was late when I was creating these fixes). I'll add another commit in a moment to sort this out.

In the mean time, I have already added two other extra commits with different fixes:

1. To allow for the test run to work on PHP 5.3 in anticipation of PR Restore php 5.3 support #51 being merged. See Restore php 5.3 support #51 (comment)
2. I noticed that the Phar was quite large for this relatively small project. This was due to the Phar including all the dev dependencies, which shouldn't actually be included.
   I've fixed that now as well, resulting in the size of the generated Phar going down from 1.5 MB to 72 KB and still working as expected. 😁

[jrfnl]

Cs2pr for linting the Parallel Lint code (integration tests)

I've done a number of test runs with cs2pr for the integration tests and have in the end decided to remove the commit which added it.

Reason being: if there is a linting error in the Parallel Lint application, the checkstyle output will never get generated correctly anyway, so the cs2pr reporting would not work, no matter what.

Part of the reason for this is the current setup with all multiple classes in each file, causing parse errors to affect the application as a whole, even when the code containing the parse error would not be run.

It might be worth it to revisit this after PR #45 has been finished and merged, but for now, adding cs2pr for the linting of the code in this application itself would never work.

And due to | filtering, the second command won't even trigger the error code to shell.
note, cs2pr itself may trigger error if it finds error in input, haven't checked that.

@glensc It does, so that is not an issue.

Test execution order

I also had another critical look at the execution order of the test commands based on the feedback.

My first suggestion does not work, but setting the first test as continue-on-error does yield the requested results.

I agree with @roelofr's suggestion here, though have ended up taking it one step further.

Please see the commit description of commit 194a080 for full details.

I've also updated the PR description to match with the changes now made.

[glensc]

#54 was merged, rebase time

[grogy]

honestly, why did anyone ever think making that the default was a good idea

I mean that the author of Nette Tester thinks about super-easy start to testing

---

Great job! PHAR is reduced from 1580kB to 73kB.

Please, after merger #45 try again to add cs2pr. I did not know the tool, but it looks interesting.

---

@glensc i will rebase and merge ;)

[jrfnl]

I mean that the author of Nette Tester thinks about super-easy start to testing

I hope that was meant sarcastically, but hey, if not: luckily there is room for different opinions in open source.
Considering how much time both @roelofr and me spend on trying to figure out why the heck all extensions were being ignored, I stand by my opinion ;-)

[glensc]

As the system php.ini was not being used, no extensions were loaded, which was causing the errors.

This is a change that was introduced in Nette Tester 2.0.0. As of that version, you need to always tell the Nette testing framework explicitly which php.ini file to use, -C tells it to use the system-wide php.ini, with -c you can specify a path to a php.ini file and by default no php.ini is used.
(honestly, why did anyone ever think making that the default was a good idea ?!!?)

I can speak from my personal experience, I've used php -n in my Linux Distribution, which has a highly modularized PHP, every extension being a shared module.

so, if you run your tests, you want to "remove" unwanted extensions, this is easy by skip loading php.ini after all, so .ini fragments with extension=spl.so, etc are not loaded, and you are left with minimal PHP modules. and if your tests run in a way that PHP interpreter is started for each test, this makes up a significant time-saver.

This approach did not work well, as it also disabled error reporting and other useful defaults.

And I think it's not relevant anymore, as you can fire up the container with PHP configured exactly for you. I.e the PHP environment is cattle, no longer a pet.

Search "Pets vs Cattle DevOps Concept" to read more.

This lead to for fixes for such environments:

• skiplintprocess: do not skip php.ini loading JakubOnderka/PHP-Parallel-Lint#80
• phingofficial/phing@8cfdd3a (the ticket reference is trac, not github)

also, you can probably find up the nettle tester 2.0 commit and/or pull request/issue, which may be describes the reasoning behind, instead of speculating :)

[jrfnl]

@glensc Oh, I totally agree that it is a good idea to have the option to run without loading the php.ini file, it's just very contra-intuitive for that to be the default.

I did look at the commits when I was working on fixing it, but don't remember seeing the reasoning for it. Then again, I didn't search too hard as, more than anything, I was just glad to have found the cause of the issue & the solution to getting the tests running again.

[ktomk]

it's just very contra-intuitive for that to be the default.

at the first time, it bites. but if you reduce the runtime environment it normally takes away side-effect problems en masse so after the first "hoppla the gap" it is then merely a straight forward ride and things then break in smaller increments, easy to handle in the gallop.

for cli utilities, another one is to strip down the environment and then see what happens. env is a good utility to do that. yes, it breaks things ;). but isn't testing all about breaking things?

containers? they need to be build first so should only be a second thought. system is already there, stripping the runtime (configuration) is pretty viable and straight forward IMHO.