* Tests/reorganize: move utf8CharBoundary test to own file
* Utf8CharBoundaryTest: reorganize to use data providers
This:
* Renames the test method and moves the description to the docblock.
* Decouples the test from the PHPMailer `TestCase` as it doesn't need the complete `set_up()` and `tear_down()`.
* Moves the test cases to a data provider.
* Adds a `@covers` tag.
* Utf8CharBoundaryTest: add `@todo` reminder
This really could do with some more test cases to properly cover all paths and branching in the method.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move address splitting tests 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`.
* ParseAddressesTest: reorganize the testAddressSplitting() test
This commit:
* Adds two new test methods, one for testing address splitting using the PHPMailer native implementation, one for testing address splitting using the IMAP implementation.
Both these methods use the same data provider.
* Adds a PHPUnit `@requires` tag for the IMAP extension to the IMAP version of the test.
* Adds a type check `assertIsArray()` before doing the content checks on the method return value.
* Changes the tests to do a more detailed `assertSame()` test instead of the previous `assertCount()` and `assert[Not]Empty()` checks.
* Sets up the data provider to have the detailed output arrays for comparison.
The actual test cases in the rewritten test are the same as were previously being tested.
* ParseAddressesTest: replace remaining test by dataprovider entry
The `testImapParsedAddressList_parseAddress_returnsAddressArray()` and the `testImapParsedAddressList_parseAddress_returnsAddressArray_usingImap()` tests were largely duplicates of each other, with the only real difference being the value of the second parameter passed to the `PHPMailer::parseAddresses()` method (`$useimap`).
This commit:
* Removes these two tests in favour of adding the test case to the data provider added in the previous commit.
In effect, the same test case is still being tested just as thoroughly now, just with less (test) code.
* ParseAddressesTest: add two more (invalid) test cases to `dataAddressSplitting()`
* PHPMailer::parseAddresses(): bug fix
If invalid email addresses are passed to any of the IMAP functions - in this case to `imap_rfc822_parse_adrlist()` - IMAP may generate error notices or warnings.
If nothing is done with these, they will be displayed at PHP shutdown.
The notice will look something like this, for instance:
```
Notice: Unknown: Missing or invalid host name after @ (errflg=3) in Unknown on line 0
```
This bug was exposed by the newly added unit tests. You should be able to see the issue if you run the tests over the previous commit.
As PHPMailer is not interested in invalid addresses, we can just ignore these notices and errors, but we *do* have to _clear_ them to prevent the notices from being thrown.
Refs:
* https://www.php.net/manual/en/function.imap-rfc822-parse-adrlist.php
* https://www.php.net/manual/en/function.imap-errors.php
* https://github.com/ddeboer/imap/issues/308
* https://stackoverflow.com/questions/3378469/how-to-get-rid-of-error-messages-with-phps-imap-fetchstructure
* ParseAddressesTest: reorder the test cases in `dataAddressSplitting()`
... to a slightly more sane order for humans to follow and see at a glance what is being tested..
* ParseAddressesTest: add `@covers` tags
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move message ID test to own file
* GetLastMessageIDTest: 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.
* GetLastMessageIDTest: split into separate tests
* GetLastMessageIDTest: reorganize to use data providers
While initially still only addressing the original test cases, using a data provider here will allow for adding additional test cases more easily.
* GetLastMessageIDTest: add additional tests
Adding additional tests based on the code this is supposed to be testing.
**Note**: I've put the test cases in the "valid"/"invalid" data providers based on the current reality.
Some of the test cases I've added to the "valid" data provider _might_ actually be **_invalid_**. If that's the case, the regex used on line 2554 needs adjusting to account for them (after which the test cases can be moved to the "invalid" data provider).
* GetLastMessageIDTest: add `@covers` tags
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move line length detected tests to own file
* HasLineLongerThanMaxTest: various test tweaks
Minor test tweaks:
* Add `@covers` tag.
* Inline comment punctuation.
* Minor code readability tweaks.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move ICal tests to own file
* ICalTest: reorganize to use data providers
These three tests were 99% duplicate code. Using a data provider removes the duplication, while the actual tests being run are still 100% the same, including same test count and assertion count.
* ICalTest: add additional tests
... to ensure all valid methods are checked.
Adding these extra tests is a breeze now with the data provider setup.
* ICalTest: various test tweaks
Minor test tweaks:
* Add `@covers` tag. **_<- is this correct ?_**
* Minor comment punctuation.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move OAuth test to own file
This test uses the `Yoast\PHPUnitPolyfills\TestCases\TestCase` instead of the `PHPMailer\Test\TestCase` base class as it doesn't need the `set_up()` or `tear_down()` methods from the PHPMailer base test class.
* OAuthTest: various test tweaks
Minor test tweaks:
* Add `@covers` tags.
* Add "failure" messages to assertions.
* Minor comment punctuation.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move POP before SMTP tests to own file
This also removed the `$pids` property and handling from the PHPMailer base `TestCase` and moves this to the POP test class.
As this test now does not actually need an instantiated PHPMailer object, this class extends the `Yoast\PHPUnitPolyfills\TestCases\TestCase` instead of the `PHPMailer\Test\TestCase`.
* PopBeforeSmtpTest: move `@group` tags
... to the class level and remove them from the individual test functions.
* PopBeforeSmtpTest: various test tweaks
Minor test tweaks:
* Add `@covers` tag at class level.
* Inline comment punctuation.
* Minor code readability tweaks.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move DKIM related tests to own file
* DKIMTest: stabilize removal of private key file
When an assertion in a test fails, the rest of the code within the test method is no longer executed.
This means that for a test which - like three out of the five DKIM test - creates a file, the test "clean up" via `unlink()` is also no longer executed.
It also means that one test may - unintentionally - interfere with the results of another test due to the file still existing, while it is expected to have been removed.
Aside from that, the `$privatekeyfile` variable used in the various test is the same everywhere, but not consistently used in the tests, as the `'dkim_private.pem'` is hard-coded in multiple places, which decreases maintainability.
This commit fixes both these issues by:
* Declaring a class constant with the target private key file name for use in the test method.
* Implements use of this constant throughout the tests.
* Removes the `unlink()` call from the individual tests, in favour of executing it via the test `tear_down()`, which should still be executed, even when a test has failed.
Note: as the file also contains two tests which do not create the private key file, but for which the `tear_down()` would also be executed, the `unlink()` call is wrapped in a `file_exists()`.
* DKIMTest: move `@group` tags
... to the class level and remove them from the individual test functions.
* DKIMTest: add missing `@requires` tags
Three out of the five tests actually require the `openssl` extension. Only one was so marked.
* DKIMTest: various test tweaks
Minor test tweaks:
* Rename a test to a more specific name to allow for easier test filtering via PHPUnit.
* Add `@covers` tags (Needs review!)
* The `@see` tag is indented for code elements. For external links, the `@link` tag should be used.
* Inline comment punctuation.
* Minor code readability tweaks.
* DKIMTest: add two additional tests
... to cover the "open SSL" not available case.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* Tests/reorganize: move email transport tests to own file
* MailTransportTest: various test tweaks
Minor test tweaks:
* Add `@covers` tags (Needs review! - especially the `testMailSend()` method seems to do more than it should)
* Check if test skipping is necessary at the start of a test method.
* Add "failure message" for each assertion in tests with multiple assertions.
* Tidy up inline comments.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* 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
Juliette
Marcus Bointon
Léon Melis