PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
549 stars 36 forks source link

Add initial integration test setup and first few tests #153

Closed jrfnl closed 2 years ago

jrfnl commented 2 years ago

Introduction

This PR creates an initial test framework to allow for creating and running integration/end-to-end tests for this plugin.

The framework as I have set it up:

The tests included in this PR are largely the tests which were already previously being run via the GH Actions workflow, though they are now encapsuled within a PHPUnit based framework and will be run via GH Actions against all supported PHP version, multiple versions of Composer and on multiple OSes.

A lot more tests are still needed, but those should be relatively straight-forward to add once this initial framework is in place and I have already started preparing some follow-up PRs to be pulled once this PR has been merged.

āš ļø This PR depends on PRs #147 and #152 both being accepted and merged.

šŸ‘‰šŸ» As this is a huge PR, I have split it up into atomic commits telling the "story" and I would strongly recommend reviewing this PR by looking at each commit separately.

Proposed Changes

Tests: create initial setup

Note: this test setup presumes that the proposal to drop support for PHP 5.3 - see issue #145 - will be accepted as the PHPUnit Polyfills require PHP >= 5.4.

Refs:

TestCase: add helper methods for setup/teardown

As most tests will need to run a composer install, tests should be run in separate, temporary directories.

With that in mind, I'm adding two helper methods to the base TestCase class:

Notes:

Includes a static TestCase::onWindows() helper method to determine whether or not the tests are being run on Windows.

TestCase: add helper method for on-the-fly composer.json creation

This adds a test helper method, which, given an array with a Composer configuration and a target location, will on-the-fly create a composer.json file for use in a test.

The method will automatically add two additional settings to the composer.json file:

By adding these keys automatically, this kind of "noise" is not needed in the actual test classes.

The method is static so as to allow for it to be used in setUpBeforeClass()/tearDownAfterClass() fixtures.

Tests: create zip package artifact for the current plugin code

In normal circumstances, a composer install will download a version of the plugin from Packagist. This will not work for the tests however, as the development version of the plugin being tested will generally not be available on Packagist.

With this in mind, we need a work-around to ensure that the tests are run with the current version of the plugin as per the last commit in the current branch.

I've implemented this by using the repositories - artifact feature of Composer.

This works as follow:

Notes:

Ref: https://getcomposer.org/doc/05-repositories.md#artifact

TestCase: add helper methods to run CLI commands and custom assertions

This commit adds six additional methods to the base TestCase class:

A number of these methods have been made static to allow for them to be used in conjunction with setUpBeforeClass() fixtures.

Tests: add TestListener to log and display CLI output for failing tests

While not necessarily the most pretty solution (output is displayed in the middle of the progress report), this allows for access to the composer.json content, the CLI commands and their output for any test which failed, which will be helpful when debugging test failures, especially in CI.

Tests: add initial baseline test

These tests verify:

These tests will be run in both a Composer global as well as a Composer project local environment.

The tests use a dataprovider which will randomly select two PHPCS versions compatible with the PHP version on which the test is being run + PHPCS dev-master + PHPCS 4.x (dev version for the next major).

As which PHPCS versions are compatible with which PHP versions needs quite some logic, a separate helper class PHPCSVersions is introduced, which encapsulates that knowledge.

The PHPCSVersions class contains a number of (static) methods as helpers. These can be used in any test class:

Generally speaking, the dev-master and 4.x branch will not be included in the retrieved ranges, but can be specifically requested by passing optional parameters.

This PHPCSVersions class will need semi-regular updates when new versions of PHPCS are released and new PHP versions are released.

Tests: move test with external standard from CI to test class

This moves the test which was being run via GH Actions to a test class in the integration test suite.

Differences between the previous and new setup:

Includes:

Tests: set up a fake PHPCS standard as a fixture and create an artifact of it

The tests in the RegisterExternalStandardsTest class contain quite some work-arounds to handle changes made over time to the external PHPCompatibility standard.

The PHPCompatibility standard was chosen for use in the tests as it is one of the rare standards which still supports both PHPCS 2.x as well as 3.x.

All the same, to improve the stability of the tests and to simplify the actual test code, using "fake" PHPCS standards set up as fixtures as part of this test suite will make life easier.

This commit creates the initial setup for that, by:

RegisterExternalStandardsTest: switch out PHPCompatibility for the test fixture

This replaces the use of the PHPCompatibility standard in the RegisterExternalStandardsTest tests with the use of the dummy-subdir fake standard fixture.

RegisterExternalStandardsTest: simplify check whether PHPCS can run with the standard

To check whether PHPCS can run with the standard, previously, a scan on a simple file was done, with this file being created on the fly for each test.

This commit replaces that check with running the "explain" command.

Advantages:

:new: BaseLineTest: selectively skip the tests

... in a very specific combination of circumstances - Windows + Composer 1.x + PHP 5.5 + PHPCS dev-master + no external standards -, as the Composer logs in that case, don't always show the "Running PHPCodeSniffer Composer Installer" message.

As it is such a specific combination, which will probably be rare to encounter in real life, I'm proposing to skip the tests for that combination instead of spending lots of time trying to debug this (for now).

šŸ†• TestCase: add willPluginOutputShow() method

In very select circumstances (PHP 5.5 with Composer 1.x on Windows) and sometimes even only with select PHPCS versions, the plugin output will not be shown in the transcript from Composer, even though the plugin does actually run.

Instead of skipping those tests completely, I'm suggestion to selectively skip the output expectation for the composer install/update run instead. That way we can still verify that the plugin has actually done its job by checking that paths have been registered with PHPCS.

I would recommend to only implement the use of this method for those select tests where we have actually seen failures due to this Composer bug and to just leave the other tests alone.

Related Issues

Closes #9

jrfnl commented 2 years ago

Note: the test failure on PHP 5.5 with Composer v1 on Windows is a really weird one. It doesn't fail on every run, just most of the time and it only seems to fail for PHPCS dev-master, which makes this even weirder. I've been trying to figure out the cause, but haven't been able to get to the bottom of it so far.

As it is a very specific combination of circumstances and one which will probably be rare IRL nowadays, we may need to decide to just skip the test for that specific combination.

Potherca commented 2 years ago

Review planned on: 2022-02-04.

jrfnl commented 2 years ago

Note: the test failure on PHP 5.5 with Composer v1 on Windows is a really weird one. It doesn't fail on every run, just most of the time and it only seems to fail for PHPCS dev-master, which makes this even weirder. I've been trying to figure out the cause, but haven't been able to get to the bottom of it so far.

As it is a very specific combination of circumstances and one which will probably be rare IRL nowadays, we may need to decide to just skip the test for that specific combination.

I've added a commit to skip the test which fails intermittently for now.

jrfnl commented 2 years ago

Note: to make this PR mergable, the required checks for the branch (branch protection settings) will need updating as well.

jrfnl commented 2 years ago

I've made a minor update to the PHPCSVersions class to ensure it is in-line with the update I made to PR #152.

jrfnl commented 2 years ago

Rebased without changes to get it past the merge conflicts.

jrfnl commented 2 years ago

Oh joy.. the new assignment operator alignment sniff was throwing some issues. Fixed up now in the respective commits. No changes other than those whitespace fixes.

jrfnl commented 2 years ago

FYI: push was to make a tiny formatting tweak to the README for the fixtures, no textual or functional changes.

jrfnl commented 2 years ago

Added one more commit to the PR as I found that some of the other tests I'm working on will also need to selectively skip a certain output expectation. Abstracting the condition to a method will make it more easily re-usable in those other tests.

jrfnl commented 2 years ago

Thanks for the extensive review @Potherca ! I've left replies everywhere relevant and adjusted those things for which there is no discussion.

Potherca commented 2 years ago

As all my points have been addressed and all questions have been answered, this is good to merge if you are content with it. :+1:

Does #147 have to merged first?

jrfnl commented 2 years ago

As all my points have been addressed and all questions have been answered, this is good to merge if you are content with it. šŸ‘

I am. āœ”ļø

I do have some follow-up PRs with tweaks, but didn't want to make the review of this PR harder than it would already be, so I will pull those changes + additional tests in separate PRs once this PR has been merged.

You can expect some ~15 follow-up PRs based on what I have lined up locally so far.

Does #147 have to merged first?

It does. As 0.7.2 has been released now, I've moved that one from draft to "ready for review". Once #147 has been merged, I'll mark this as ready (and if needs be rebase it without changes to get past potential imaginary merge conflicts).

The branch protection settings will need updating for both PRs, so may be easiest to just do that once after this one has been merged.

@mjrider Did you still want to have a look at either of these PRs before we merge them ?

jrfnl commented 2 years ago

This PR depends on PRs https://github.com/PHPCSStandards/composer-installer/pull/147 and https://github.com/PHPCSStandards/composer-installer/pull/152 both being accepted and merged.

Rebased now the prerequisites have both been merged & marking this ready for review/merge.

jrfnl commented 2 years ago

Note: to make this PR mergable, the required checks for the branch (branch protection settings) will need updating as well.

I've updated the branch protection settings to match the new builds and removed the old builds. PR should be mergable now.

jrfnl commented 2 years ago

Thanks for getting this merged. I'll start pulling the follow up PRs.

jrfnl commented 2 years ago

Related to #92