So far, these methods were only tested in the most perfunctory manner.
The additional tests this commit introduces, test all aspects of the methods as well as documents the current behaviour of the methods.
Take note of the "text merging"/readability issues for the SMTP error messages. There may be room for improvement there.
... by a much simpler test which effectively tests the same thing, i.e.:
* No errors to start with.
* Trigger an error.
* Verify that `PHPMailer::isError()` returns `true`.
* Verify that the error message is as expected.
What with the previous commit adding a requirement for the `Mbstring` extension to the existing tests, it becomes clear that the "Mbstring extension not available" code path was not covered by the tests.
This commit fixes that by:
* Adding two new test methods which explicitly expect the Mbstring extension to **not** be available.
* Changing the data provider "expected(Imap)" keys.
The `expected` key in the array will now be an array of arrays.
A `default` key can be used for when the output across configurations will be the same.
Any differences across configurations can be provided in separate sub-keys of the `expected` array, using the `native+mbstring`, `imap+mbstring`, `native--mbstring` and/or `imap--mbstring` keys, which match the four test methods which are now in place.
Additionally, an `native` and/or `imap` key can be used for setting the output expectations for the two native implementation or the two IMAP implementation tests, if the Mbstring extension makes no difference.
Follow up to #2266
Bug fix
In both "arms" (imap vs native implementation) of the `PHPMailer::parseAddresses()` method, the `mb_decode_mimeheader()` function is used to decoded a (utf-8) encoded name.
In the IMAP "arm", a check was in place for the Mbstring extension being available before using it. This check was missing from the "native implementation" "arm".
Existing Tests
This also means that both currently existing tests have a requirement for the MbString extension being available. This was previously not made explicit in the tests.
Fixed now.
... which should be handled correctly based on the code in the method under test.
With these additional test cases, the method now has 100% code coverage and is fully tested.
Using a regex assertion with arbitrary input data which is not regex escaped, makes this test suspect.
From the looks of it, the test _should_ be testing that the output is the _same_, so let's use that assertion.
This commit:
* Adds a new `testFailToAttach()` test method to test the behaviour of the `PHPMailer::AddStringEmbeddedImageTest()` method when the `PHPMailer` class has been instantiated with `$exceptions` disabled.
* This new test method uses the same data provider - introduced in the previous commit - as the `testFailToAttachException()` method.
This commit:
* Renames the `testStringEmbeddedImageEncodingException()` test to `testFailToAttachException()`.
* Reworks the test to use a data provider.
* Adds testing of the exception message to the `testFailToAttachException()` method.
This commit:
* Improves the test name and the description in the docblock.
* Replace a redundant condition and "forced" failure assertion with an assertion actually testing the result of the method call.
* Removes the redundant `return` - if an assertion fails, the rest of the code within the test method will not be executed anyway.
* Minor inline comment tweaks.
The actual "attaching" of the string attachment happens within `createBody()` which is called from `preSend()`, so this test doesn't actually need to call `send()`.
So far, this method did not have dedicated tests.
The test file this commit introduces, tests nearly all aspects of the method as well as documents the current behaviour of the method.
There is one particular test case missing. This is annotated in the class docblock.
While this method is quite simple, testing it separately means that the tests for methods _using_ this method don't have to _also_ test the functioning of this method, which means they can be more focussed on their own logic.
So far, this method did not have dedicated tests.
The test file this commit introduces, tests all aspects of the method as well as documents the current behaviour of the method.
While this method is quite simple, testing it separately means that the tests for methods _using_ this method don't have to _also_ test the functioning of this method, which means they can be more focussed on their own logic.
This commit:
* Adds a new `testFailToAttach()` test method to test the behaviour of the `PHPMailer::addStringAttachment()` method when the `PHPMailer` class has been instantiated with `$exceptions` disabled.
* This new test method uses the same data provider - introduced in the previous commit - as the `testFailToAttachException()` method.
This commit:
* Renames the `testStringAttachmentEncodingException()` test to `testFailToAttachException()`.
* Reworks the test to use a data provider.
* Adds testing of the exception message to the `testFailToAttachException()` method.
The actual "attaching" of the string attachment happens within `createBody()` which is called from `preSend()`, so this test doesn't actually need to call `send()`.
This commit:
* Adds an `exceptionMessage` index to the `dataFailToAttach()` data provider.
* Renames the `testEmbeddedImageEncodingException()` method to `testFailToAttachException()`.
* Sets the `testFailToAttachException()` method up to use the `dataFailToAttach()` data provider.
* Adds testing of the exception message to the `testFailToAttachException()` method.
With this change, the "fail to attach" test cases are now fully tested for both a `PHPMailer` instance without exceptions enabled, as well as for an instance _with_ exceptions enabled.
The test code remaining in the `testEmbeddedImage()` constitutes one test.
This commit:
* Renames the test method and improves the description in the docblock.
* Removes the redundant call to `PHPMailer::clearAttachments()`.
This call was previously needed as multiple situations were being tested in one test method.
Now each test case has its own test method, the call to `PHPMailer::clearAttachments()` is no longer needed as each test method will receive a fresh, clean instance of the `PHPMailer` class.
* Replace a redundant condition and "forced" failure assertion with an assertion actually testing the result of the method call.
* Removes the redundant `return` - if an assertion fails, the rest of the code within the test method will not be executed anyway.
The test case when a file was attached without explicitly adding a filename wasn't actually being tested at all as no assertion was used.
This commit:
* Moves that particular test case to a separate test method.
* Adds relevant assertions to actually test the test case.
The "failure" case when a non-existent file was being passed, wasn't actually being tested at all as no assertion was used.
This commit:
* Moves the particular failure test case to a separate test method with a data provider (to allow for more failure test cases to be added).
* Uses an assertion on the call to `addEmbeddedImage()` to actually test that the method return a failure state.
* Verifies that no attachment for an inline image was added by adding a second assertion with a call to `PHPMailer::inlineImageExists()`.
The actual "attaching" of the images happens within `createBody()` which is called from `preSend()`, so this test doesn't actually need to call `send()`.
* Maintains (largely) the same test cases.
* Prevent one failing assertion hiding a potential second failure.
* Makes it easier to add additional test cases in the future.
Note:
This removes the intermittent calls to `clearCustomHeaders()` from this test. This is now tested via a separate method and as each test case will receive a fresh instance of the `PHPMailer` class, there is no need to clear the set custom headers between tests.
The line length adjustments are executed within `createBody()` which is called from `preSend()`, so this test doesn't actually need to call `send()`.
I also wonder if this test can be further improved by having more targeted tests for `PHPMailer::hasLineLongerThanMax()` and whether these more extensive tests can be included in a test class which is more targeted at the `createBody()` and/or `preSend()` methods, but that is for later.
Previously, the test didn't actually test whether the wordwrapping had been applied, just that the message was (pre)send successfully.
Changing the assertions to actually test that the wordwrapping has been correctly applied.
For this test, there is no need to actually try to _send_ the message, we just need to make sure that `setWordWrap()` is triggered, which can be done by calling `preSend()` instead of `send()`.
This new test method covers a range of cases where the `PHPMailer::punyencodeAddress()` method should (and does) return the original input value unchanged.
This test does not require the `mbstring` extension or `idn_to_ascii()` function to be available, which is why it has been set up as a separate test with a separate data provider.
The "fakefunctions" are all nice and dandy to get past the `idnSupported()` check, but if either of these functions is not _really_ available and therefore doesn't behave as expected, the test would still fail as the expected output of the `PHPMailer::punyencodeAddress()` function would not match.
In other words, this test should not use the `fakefunctions`, but should have a hard requirement for the `mbstring` extension (for the `mb_check_encoding()` and the `mb_convert_encoding()` function calls) and a check for the `idn_to_ascii()` function.
Follow up on 2389 and 2412
Now the base `TestCase` has been simplified and only presets the bare minimum of properties in the `PHPMailer` class, the `Utf8CharBoundaryTest` can actually use it.
The `PHPMailerTest` class contained a `testBootstrap()` method to verify that the `testbootstrap.php` file exists as the first test in the class.
As the order in which tests are run is not predefined, this is not reliable.
Additionally, the check for the `testbootstrap.php` file is checking a pre-requisite for tests using the `PHPMailer::send()` method, so it would be better to verify via a condition in the `set_up()`.
Now, using such a condition, there is choice: the test can either be marked as "skipped" when the `testbootstrap.php` file can not be found, or be marked as an "error".
As the tests _should_ run, skipping them would hide an error in the dev-user test setup, so showing these tests as errors seems more appropriate.
To that end, the `testBootstrap()` method has been removed and a check for the `testbootstrap.php` file has been added to the `SendTestCase::set_up()` which will now throw an appropriate `Exception` when the file is not found.
Oops... also removes a stray `parent::set_up()` at the start of the local `set_up()` which should have been removed in 218fd13c88
Note: as the `PHPMailer::generateID()` method is `protected`, a round-about way of testing this is needed, but this test does verify the functioning of the method.
Take note of the notes in the test docblock - the GH Actions scripts running CI, should make sure that each of these scenarios is encountered/tested.
This will be addressed in a separate PR at the end of this round of test changes.
Minor test tweaks:
* Add `@coversNothing` tag.
* Rename the first test (add a number).
* Use `@link` instead of `@see` for links.
* Minor comment punctuation.
The `TestCase::checkChanges()` method is a way of exposing what properties in the `PHPMailer` class have a changed value compared to their default value in a particular test situation. The method is used for debugging tests.
As things were, the `TestCase::checkChanges()` method would check against a limited set of hard-coded values to determine whether the default value of a property has been updated.
This is unstable as:
1. Default values may change in the `PHPMailer` class and the values within this method would need to be updated to match, which is easily forgotten.
2. New properties may be introduced in the `PHPMailer` class and be relevant to this debug changelog.
Again, it would require manually adding these new properties to this method to start tracking them.
3. Property values may be changed in the `set_up()` method, which would be a "known change" for a certain test.
In part such "expected" changes were taken into account in this method based on the previously hard-coded setting changes in `set_up()`.
With the logic for the property setting from the `set_up()` method now being more flexible, the pre-setting of properties having been reduced to the bare minimum, but also allowing individual test clases to set their own additional changes, keeping track of what is a "known" change by checking against hard-coded values is no longer stable.
With this in mind, I propose to make the `TestCase::checkChanges()` method dynamic.
To that end, this commit:
* Retrieves the default values of all properties of the `PHPMailer` class via the PHP native `get_class_vars()` function.
* Will automatically check for changes in *all* properties, with only a limited set of _exclusions_, effectively changing the changelog from an "inclusion list" to an "exclusion list".
A select list of properties is excluded from being listed in the changelog via the `TestCase::$changelogExclude` property.
See the inline documentation in the property for the reasoning behind excluding certain properties from the changelog.
* The value of static properties will always be compared to their default value as registered in the `TestCase::$PHPMailerStaticProps` method and will be listed when different.
_Note: as documented, this list has to be hard-coded due to Reflection (as well as `get_class_vars()`) not handling default values for static properties correctly._
* The value of non-static properties will be compared to both the known changes made in the `set_up()` method and if the property was not changed in `set_up()`, to their default value. The property will be listed in the changelog when the value is different from the "expected" value, i.e. not a known change from `set_up()` and not the default value.
In addition to this, the representation of the properties will now be created via `var_export()`, which allows for listing `null` and boolean values as well.
The `SendTestCase` gets the values of the properties to be set from the `testbootstrap.php` file.
This introduces a `private` property to map the field names used in `$_REQUEST` to the properties in the `PHPMailer` class and adds logic to the overloaded `set_up()` method to fill the `$propertyChanges` TestCase property. The actual setting of the properties in the `PHPMailer` instance is deferred to the underlying `TestCase` parent class.
Includes adding support for presetting the `bcc` value for feature completeness.
Overloading and/or adding to the `$propertyChanges` array from concrete test cases is, of course, supported, so if individual tests need additional presetting of properties, the same logic as mentioned in the previous commit can be used.
After some investigation, it turns out that barely any of these properties are actually needed for the `PHPMailer::preSend()` method to succeed.
This commit removes all presetting of properties for the PHPMailer instance created by the `PreSendTestCase`, save for the bare minimum.
Overloading and/or adding to the `$propertyChanges` array from concrete test cases is, of course, supported, so if individual tests need additional presetting of properties, the same logic as mentioned in the previous commit can be used.
This commit makes the property setting in the `TestCase::set_up()` more flexible by combining an overloadable property `$propertyChanges` and a `foreach` loop to set the actual property values.
Concrete test classes can either overload the `$propertyChanges` property with their own version or can add to the default setup using the following pattern:
```php
protected function set_up()
{
$this->propertyChanges['additional_key'] = 'value';
// Add more properties...
parent::set_up();
}
```
The `TestCase::set_up()` was setting quite a number of properties in the `PHPMailer` class.
This makes testing more difficult for the following reasons:
1. The tests can no longer presume the properties in the `PHPMailer` class will have their default values
This means that tests are not "transparent" (clearly show what is being tested), nor isolated (only target what is specifically being tested).
2. Any changes to the values set in the `set_up()` method may have a ripple effect and create a need for individual test expectations to be adjusted.
3. As the `set_up()` is changing a number of the properties using methods in the `PHPMailer()` class and methods called during the `set_up()` are included in code coverage visualizations, code coverage cannot fully be trusted and it is more difficult to verify that each piece of code has tests covering that code path.
With this in mind, I'm proposing splitting the `TestCase` into three distinct abstract `TestCase`s:
* A basic `TestCase` containing the utility methods and a minimal `set_up()` and `tear_down()`.
* A `PreSendTestCase` for use with tests using the `preSend()` method which requires a few properties to be set.
* A `SendTestCase` for use with tests actually testing the sending of mail using the `send()` method, which needs yet more properties and uses the `testbootstrap.php` file to retrieve the values of those variables.
This commit executes the initial split. Follow-on commits will streamline this further.
Includes adjusting the `TestCase` being extended for select existing unit test classes.
... which should be handled correctly based on the code in the method under test.
With these additional test cases, the method now has 100% code coverage and is actually tested quite fully.
* Split the test into two tests with each a dedicated data provider
* Maintains the same test code and test cases.
* Makes it easier to add additional test cases in the future.
**Note**: the data set description may need some improvement, I've described them based on my best guess of what they were testing.
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`.
So far, this method did not have dedicated tests, though the `PHPMailerTest::testAttachmentNaming()` test covered this partially.
The test file this commit introduces, tests all aspects of the method as well as documents the current behaviour of the method.
Test cases largely inspired by the tests in the `PHPMailerTest::testAttachmentNaming()` method.
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`.
Note: this doesn't move the complete test from the original test file, just select parts of the test method.
Follow up on 2384.
1. This test does not actually need a regular expression to test the mime message, an `assertStringContainsString()` assertion is sufficient.
2. Remove duplicate information from the `$expected` parameter. The `Content-Type: text/calendar; method=` part will always be the same. Only the method name will change.
Including test cases with:
* Spaces in the paths.
* Incomplete file paths.
Note: the "empty string" test may need looking at.... should this return an empty array instead ?
* Replace the existing test code with two new test methods using data providers.
* Maintains the same test code and test cases.
* Makes it easier to add additional test cases in the future.
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`.
Note: this doesn't move the complete test from the original test file, just select parts of the test method.
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`.
Note: this doesn't move the complete test from the original test file, just select parts of the test method.
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`.
Note: this doesn't move the complete tests from the original test file, just select parts of two test methods.
The `PHPMailer::preSend()` method may alter the value of the _`protect static`_ `PHPMailer::$LE` property.
a9978d2079/src/PHPMailer.php (L1494-L1504)
This means that tests which rely on the value of the property being the default, may start to fail unexpectedly depending on the _order_ in which tests are being run.
I.e. if the test relying on the default value is run first, all will be fine, however, if that same test is run _after_ a test which calls `preSend()` on a non-Windows box with PHP < 8.0 and with `$this->Mailer` set to `mail`, the test may fail unexpectedly.
This commit fixes that by expanding the previously introduced "resetting of static properties" logic.
This commit:
* Expands the list of static properties in the `TestCase` class to include the `PHPMailer::$LE` property.
* Introduces two new methods to the `TestCase` class:
- `resetStaticProperties()` to reset all static properties listed in the `TestCase::$PHPMailerStaticProps` property to their default values.
- `updateStaticProperty()` to change the value of one individual static property in a class.
* The `updateStaticProperty()` method was made `public static` to allow for tests which do not need an instantiated PHPMailer object and thus extend the `Yoast\PHPUnitPolyfills\TestCases\TestCase` directly to be able to use the method as well.
If any such tests would use the method, that particular test will be responsible for resetting the value back to the default after the test.
* The resetting of the static properties has also been moved from the `set_up()` to the `tear_down()` method.
As mentioned above, these static properties may be used by both tests extending this `TestCase` as well as test extending the `Yoast\PHPUnitPolyfills\TestCases\TestCase`. With that in mind, resetting _after_ each test will be more stable, as long as tests which extend the `Yoast\PHPUnitPolyfills\TestCases\TestCase` and would change static properties clean up after themselves.
**Note**: I'm not including the `PHPMailer::$IcalMethods` property in this change as AFAICS, this property is not being changed anywhere in the package code and as the property is `protected`, it cannot easily be changed from within a test either (and if it would be changed by a test, that test would be responsible for resetting it).
Also note: due to limitations in the `Reflection` extension, a hard-coded list of the default values of the static properties will need to be maintained in the `TestCase`. This cannot be helped as the `Reflection` extension does not have a way to accurately retrieve the default value of static properties in a PHP cross-version compatible manner.
The `PHPMailer::__construct()` method has an optional `$exceptions` property to throw exceptions on errors.
Up to now, the `PHPMailer` instance created by the `set_up()` would not pass any parameters, effectively instantiating `PHPMailer` without turning on exceptions.
In a test situation it may be useful for tests to test the behaviour of a method _with_ and _without_ the exceptions option and to test that certain exceptions are thrown and throw the correct message.
With that in mind, I'm introducing a `USE_EXCEPTIONS` class constant to the `TestCase` which can be overloaded in individual test classes and will be used by the `set_up()` method to determine whether it will be instantiated with exceptions or not.
Includes introducing an overload of the class constant in one of the test class for which it would seem appropriate at this time.
Note: this does mean that the `testDKIMSigningMail()` test will show as errored instead of failed if no SMTP connection could be made, but that IMO is the more correct status anyhow.
This allows for using the `setAddress()` method in a more consistent manner (where appropriate).
Includes introducing the use of the `setAddress()` function in a few select places.
Note: I do wonder whether this method should ever be used outside of `set_up()` and `tear_down()`, but that is for further discussion and outside the scope of this commit.
While this class does not (currently) _change_ the `PHPMailer::$validator` property, it does rely on the default value being the expected default.
By resetting the value of the property before and after the class, this is safeguarded for the current tests.
Public static properties changed by individual tests were not being reset to their default value prior to the next test being run.
This could influence the test results of subsequent tests as static properties are not automatically reset when a new instance of a class is instantiated. See: https://3v4l.org/8s1RB
Luckily this didn't aversely affect the tests so far, but should be safeguarded for the future.
There are only three static properties in the `PHPMailer` class, with only one of these being `public`.
This commit introduces a code snippet which will reset any static properties as listed in the `$PHPMailerStaticProps` property of the `TestCase` to the default value, as also set in the `$PHPMailerStaticProps` property, at the start of the `set_up()` which is run before each test using the `TestCase`.
For now, this is only relevant for the `ValidateAddressCustomValidatorTest` test and only when the `testSetDefaultValidatorToCustom()` test would fail before resetting the property. All the same, having the reset in place by default will ensure that future tests which change this property won't introduce side-effects to other tests.
... in part to document the behaviour of the function, in part to actually test it.
By the looks of it more tests are still needed though to raise the code coverage of this function to 100%.
I'd also recommend adding tests with different charset settings as the current tests are all based on the default charset (`CHARSET_ISO88591`).
Also note: the "empty string" test may need looking at.... is this a bug ?
* Maintains the existing test cases.
* Prevent one failing assertion hiding a potential second failure.
* Makes it easier to add additional test cases in the future.
As this test is marked _incomplete_, no further review of the test has been done and no `@covers` tag has been added.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* 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>
* 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>
