* Tests/reorganize: move email validation using custom validator test to own file
* ValidateAddressCustomValidatorTest: reorganize test
Split the test into three tests, each testing a specific situation and use a data provider for one of the tests.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move email validation test to own file
As this test does not actually need an instantiated PHPMailer object, this class extends the `Yoast\PHPUnitPolyfills\TestCases\TestCase` instead of the `PHPMailer\Test\TestCase`.
* ValidateAddressTest: reorganize to use data providers
The original test as was, would run through a number of arrays and keep track of fails/passes, only to use an assertion at the end to check that the list of "fails" was empty.
In addition to this, the original test also contained some additional assertions which would never be run if the earlier assertion would fail. (failing assertion possibly hiding more failing (or passing) assertions).
Using data providers for these kind of data array based tests, has a couple of advantages:
1. Each data set is counted as an individual test.
2. Each test can be set up to have only one assertion.
3. When a test for a data set fails, PHPUnit just moves on to the next data set, instead of failing the test and not examining the rest of the test cases.
With that in mind, this test has now been reorganized into multiple test functions, each with one or more data providers.
In addition to that:
* Each data set in a test provider is named after the email address it provides, with optionally a prefix to show which data provider it came from.
This has two advantages:
1. When using the `--testdox` runner, the output will list each test case by name.
2. When a test fails, instead of getting a "failed with data set 65" message, you now get a "failed with data set _data set name_" message, and as the data set name is the same as the email address value, it's easy to see which test case failed.
* Each assertion now has a "failure message" attached, as the default "true does not match false" message from PHPUnit is not very descriptive.
* ValidateAddressTest: enable two out of three of the unused data sets
The original test contained three additional data sets which were *not* being tested:
* `$invalidphp`
* `$validqandc`
* `$validipv6`
The `$invalidphp` data set has now been set up as a data provider and has been added to the `testInvalidAddresses()` test.
The `$validipv6` data set has now been set up as a data provider and has been added to the `testValidAddresses()` test.
And the `$validqandc` data set has been removed after consultation with synchro.
Note: there are six test in the `$validipv6` array which are currently failing. Those have been commented out to be addressed later.
* ValidateAddressTest: add `@todo`
While the tests in this class will show that the `PHPMailer::validateAddress()` is 100% covered by tests, the tests do **not** in actual fact test all functionality properly.
To that end, I've added a recommendation in a `@todo` at the top of the class to document how these tests could be further improved in a future iteration.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* PHPMailerLangTest: rename test class to `TranslationCompletenessTest`
As the test class has been moved to a separate directory, we may as well make the class name more descriptive of what the test class actually does.
* TranslationCompletenessTest: various test tweaks
Minor test tweaks:
* Move `@group` tag up to class level.
* Add a `@coversNothing` tag as this test is more a maintainer utility/package test than a test to cover functionality in code.
* Tidy up inline comments.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: add an abstract base testcase
As a first step towards reorganizing the tests, this commit:
* Creates an abstract base `TestCase` class which can be extended by concrete child test classes and holds the generic properties and helper methods for use throughout the tests.
Based on their use, the visibility of properties and methods have been adjusted for the new setup.
* Removes the generic property and helper method declarations from the concrete test class.
* Moves the `require` statement for the `validators.php` file to a `set_up_before_class()` method in the base `TestCase`.
* Tests/reorganize: define base directory in set_up_before_class
The `$this->INCLUDE_DIR` property which points to the project root directory does not change at any time during the test run, but was being redefined for every test in the `set_up()` method.
As this is in effect a _constant_ value, let's define it as a constant in the TestCase `set_up_before_class()` method instead.
Notes:
Both actions executed in the `set_up_before_class()` method are typically things for a test bootstrap file.
However, to allow for PHPUnit to be able to run from both a Composer install as well as a Phar file, without having to create custom autoloaders, it is simpler to have the `vendor/autoload.php` file as the bootstrap file as, in that case, PHPUnit will handle the loading order and prevent loading conflicting PHPUnit files from a Composer install when running via the Phar.
With this in mind, putting these actions in a `set_up_before_class()` method is a valid compromise.
* Tests/reorganize: move actual test files to subdirectories
... leaving the test root directory to only contain test utility files.
Note: I've added a second entry for the test generated files to the `.gitignore`. Adding this entry instead of replacing the entry allows for any existing generated files in contributor clones to continue to be ignored.
At a later point in time, it could be elected to remove the original entry, once all active contributors have updated their installs and removed any stray generated files from their `test` root directories.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* GH Actions: run on PRs and allow for manually triggering
Currently the workflow only ran on `push` events, which - as forks have to enable the workflows - means that PRs could be submitted without CI having been run and you'd only see the CI results on merge.
By adding the `pull_request` event, it is ensured that CI is always run within the main repo for pull requests. This also allows for branch protection to be enabled with "required statuses".
Additionally, 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 `master` or 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
Currently the PR template asks for people to run the CS tooling.
As the PHPCS tool is also run in the test workflow and this workflow - per the previous commit - will now also be run on pull requests, we can make life easier on contributors.
The cs2pr tool allows to display the results from an action run in checkstyle format in-line in the PR code view.
This commit enables this for PHPCS, which means that the code view will now show CS violations in the PR.
Ref: https://github.com/staabm/annotate-pull-request-from-checkstyle
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests: remove unused test bootstrap file
The PHPUnit config file requires the `vendor/autoload.php` file as the test bootstrap and this file is not referenced anywhere in the code base, so this is dead code.
* Tests: apply test method naming conventions
For tests to be picked up by PHPUnit automatically, the method should start with the prefix `test`.
For differently named tests, the `@test` annotation can be used to still mark a method as a test and get PHPUnit to run it.
As the vast majority of tests use the "prefix the method with `test`" convention, this changes the names of the few tests which did not comply with that convention and removes the `@test` annotations.
* Tests: use test skipping where appropriate
In this case, the condition being tested should never be `false`, so could possibly be removed.
All the same, if the condition _would_ result in a `false`, the test would be marked as "risky" as no assertions would be run by it.
This can be avoided by using the condition to set a test skip annotation, instead of wrapping the actual test code in the condition.
* Tests: use strict assertions
PHPUnit contains a variety of assertions and the ones available has been extended hugely over the years.
To have the most reliable tests, the most specific assertion should be used.
Most notably, this changes calls to `assertEquals()` to `assertSame()`, where `assertEquals()` does a loose type comparison `==` and `assertSame()` does a strict type `===` comparison.
The only real exception to this is when comparing two objects, as in that case, the objectID will not be the same, so those should still use `assertEquals()` - or the PHPUnit 9.4.0 `assertObjectEquals()` method for comparing value objects using a callback method in the ValueObject class.
* Tests: use the correct parameter order
For PHPUnit assertions which expect an `$expected` and a `$result` parameter, the parameter order is always `( $expected, $result, ...).
While it may not seem important to use the correct parameter order for assertions doing a straight comparison, in actual fact, it is.
The PHPUnit output when the assertions fail expects this order and the failure message will be reversed if the parameters are passed in reversed order which leads to confusion and makes it more difficult to debug a failing test.
* Tests: use static closures
... when the closure doesn't use `$this`.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
The `docs` workflow to deploy the GH Pages website is run on pushes to `master`, but that includes pushes to `master` in forks, which obviously can't deploy to the GH Pages site.
This means that in forks (and there are nearly 9000 of them), this workflow will always fail, while in reality, it shouldn't be run in the first place.
So, I'd like to propose making this small change, which _should_ prevent the `docs` workflow from being run on forks.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
From the PHP 8.1 changelog:
> `htmlspecialchars()`, `htmlentities()`, `htmlspecialchars_decode()`,
>` html_entity_decode()` and `get_html_translation_table()` now use
> `ENT_QUOTES | ENT_SUBSTITUTE` rather than `ENT_COMPAT` by default. This means
> that `'` is escaped to `'` while previously it was left alone.
> Additionally, malformed UTF-8 will be replaced by a Unicode substitution
> character, instead of resulting in an empty string.
Ref: 28a1a6be08/UPGRADING (L149-L154)
If effect this means that the output of the above mentioned functions may be different depending on the PHP version and the passed text string, unless the `$flags` parameter is explicitly passed.
This patch proposes to add the old default to the one affected function call in PHPMailer, though most other flag combination choices would be just as valid, as long as a `$flags` parameter is passed.
Ref: https://www.php.net/manual/en/function.htmlspecialchars.php
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
On PHP 8.1, passing `null` to `trim()` generates a `trim(): Passing null to parameter #1 ($string) of type string is deprecated` notice.
As the `$value` is optional and may not even get set via the `name:value` parsing, the code as was, was causing this notice to be thrown.
This fix is covered by the existing unit tests and was exposed when running the tests on PHP 8.1 with `error_reporting` set to `E_ALL`.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
While working on 2363, I noticed that the code sniffer run was using PHP 8.0, but Composer still allowed for a PHP_CodeSniffer version to be installed which is not fully compatible with PHP 8.0.
Note: there are still two known incompatibilities with PHP 8.0 in PHPCS 3.6.0, but everything else has been fixed.
Ref: https://github.com/squizlabs/php_codesniffer/releases
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* GH Actions: start testing against PHP 8.1
The first alpha of PHP 8.1 has been released, so now seems like a good time to start running the tests against PHP 8.1.
For now, I've configured it to allow builds against PHP 8.1 to fail, while PHP 8.1 is still unstable.
Also: PHPUnit doesn't officially support PHP 8.1 yet, so to install PHPUnit 9.x on PHP 8.1, we need to use `--ignore-platform-reqs`, as otherwise PHPUnit 4.8 would be installed (last PHPUnit version without strict PHP version constraints).
* GH Actions: set error reporting to E_ALL
Turns out 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.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
A typical SMTP transaction ID for Haraka looks like this:
```
250 Message Queued (14490C56-76FB-4932-A59B-A8299DB2B693.1)
```
This regex will detect and extract this transaction ID
