Initial unit test setup, including tests for the Backticks sniff #70
Conversation
* Adds PHPUnit to the `dev` requirements in the `composer.json` file.
- As the PHPCS PHPUnit framework isn't compatible with PHPUnit 7 until PHPCS 3.2.3, I've chosen to explicitly only support PHPUnit 4, 5, 6 via Composer to prevent having to make extra allowances for PHPUnit 7 in the Travis script.
- Includes raising the minimum PHPCS requirement from PHPCS `3.0.2` to `3.1.0` as PHPCS `3.0.2` does not yet contain the PHPUnit `bootstrap.php` file for cross-version PHPUnit compatibility.
- Includes adding a convenience script to easily run the unit tests.
* Adds a `phpunit.xml.dist` configuration file.
* Adds a `phpunit-bootstrap.php` file to sort out the autoloading for the unit tests and to prevent PHPCS from trying to run unit tests for other installed external standards.
* Adds an abstract base test case for the security sniffs which handles setting the configuration variables for the tests.
The configuration variables are set based on the test case file name. See the explanation in the file docblock.
* Adds running the unit tests to the Travis configuration.
As the full unit test matrix can take a little while to run, I've set this up in a way that on pushes only a quick check is being run and that the full build is only run on PRs and merges to `master`.
The updated build matrix takes compatibility of PHPCS with various PHP versions into account.
Includes:
* Adding the test related files to the `.gitattributes` `export-ignore` list so they don't get shipped in the release packages.
* Adding typical PHPUnit overload/cache files to the `.gitignore` file.
The sniff would only report on the first variable found in the shell command, not on each variable. Even though there would be a notice, the level could have been `warning` as the first variable seen was a non-user input variable, while a more serious `error` for a subsequently used user input variable would not be reported. This has now been fixed by changing the check for a variable to a loop which will report a separate error/warning for each variable encountered in the command.
A backtick-shell command can be spread out over several lines. This minor change make it so the error/warning will be reported on the line containing the offending variable, not the line containing the open-backtick, as these may not be the same.
Only set variables if they are actually needed, not before.
|
The Travis build shows the tests being run and against which PHP/PHPCS version these are now run: https://travis-ci.org/github/FloeDesignTechnologies/phpcs-security-audit/builds/662885145 I can also see the Travis check in the above "merge" block. This is good as that means that, if for any future PR the unit tests would start failing due to some change, merging can be blocked. Merge blocking on Travis failures can be turned on via the repo |
|
Should we still write tests that support Drupal8 and Symfony2 when they are not really implemented in the tool yet (and we have to plan to do so afaik). This will add up to the confusion that having the folders creates right now. We might even need to delete those folders to remove that confusion. What do you think? |
|
If I understood correctly, to run the tests locally I need to do |
|
@jmarcil While the If setting They can always be brought back at a later point in time when official support would be added. In that case, I can send in a PR to remove them and adjust this PR to remove support for those options (and the related unit tests) once the PR removing those folders has been merged. |
Correct.
Yup: |
|
Okay yes, let's remove them. I can take care of the README, I have a few things I wanna tweak on it anyways. Merging this! THANKS A LOT!!!! 😄 |
This is an initial setup for the unit tests.
To allow for testing with different configuration variables -
ParanoiaModeandCmsFramework-, I've chosen to create a setup which will automatically set these variables based on the test case file name.This initial setup includes unit tests for the
BadFunctions.Backtickssniff, as well as three commits making small fixes to the sniff.Related to #57
Commit summary
CI: Initial unit test setup
devrequirements in thecomposer.jsonfile.3.0.2to3.1.0as PHPCS3.0.2does not yet contain the PHPUnitbootstrap.phpfile for cross-version PHPUnit compatibility.phpunit.xml.distconfiguration file.phpunit-bootstrap.phpfile to sort out the autoloading for the unit tests and to prevent PHPCS from trying to run unit tests for other installed external standards.The configuration variables are set based on the test case file name. See the explanation in the file docblock.
As the full unit test matrix can take a little while to run, I've set this up in a way that on pushes only a quick check is being run and that the full build is only run on PRs and merges to
master.The updated build matrix takes compatibility of PHPCS with various PHP versions into account.
Includes:
.gitattributesexport-ignorelist so they don't get shipped in the release packages..gitignorefile.BadFunctions/Backticks: add unit tests
BadFunctions/Backticks: bug fix - report on each variable
The sniff would only report on the first variable found in the shell command, not on each variable.
Even though there would be a notice, the level could have been
warningas the first variable seen was a non-user input variable, while a more seriouserrorfor a subsequently used user input variable would not be reported.This has now been fixed by changing the check for a variable to a loop which will report a separate error/warning for each variable encountered in the command.
BadFunctions/Backticks: error message line precision
A backtick-shell command can be spread out over several lines.
This minor change make it so the error/warning will be reported on the line containing the offending variable, not the line containing the open-backtick, as these may not be the same.
BadFunctions/Backticks: minor efficiency fix
Only set variables if they are actually needed, not before.