From 98e110e66f8fd3047cc0488c2ead8a2b740fdf0e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 11 Sep 2024 20:38:11 +0200 Subject: [PATCH] Tests: isolate test which fakes functions The test suite contains a `fakefunctions.php` file which is loaded by one particular test. This `fakefunctions.php` file makes sure that two functions - `idn_to_ascii()` and `mb_convert_encoding()` - will always be available, even when the `Intl` and/or `MbString` extensions are not loaded. This is problematic for two reasons: **Incorrect return type** In both cases, the return value for the faked function does not comply with the expected/documented return type, which can/will lead to unexpected behaviour of the code under test as the code under test (correctly) does not take this unexpected return type into account. * The faked `idn_to_ascii()` function always returns `true`, while the expected/documented return type of this method is `string|false`. * The faked `mb_convert_encoding()` function always returns `true`, while the expected/documented return type of this method is `array|string|false`. Refs: * https://www.php.net/manual/en/function.idn-to-ascii.php * https://www.php.net/manual/en/function.mb-convert-encoding.php **Test is not run in isolation** When this `fakefunctions.php` file is loaded and either the `Intl` or the `Mbstring` extension is unavailable, the functions are defined and stay in memory. In practice, this means that depending on the _order_ in which tests are being run, this can screw up the expected test results for _other_ tests being run _after_ the test which loaded the fake functions, as a `@requires function idn_to_ascii` annotation will no longer work as expected (and skip the test), but will now assert that the function is available and _run_ the test, which will lead to test failures elsewhere as the function doesn't work as expected (because it is faked and returning an incorrect return type). To address this problem, there are a couple of questions which need to be asked, assessed and possibly addressed in a follow up issue: 1. What is this method really testing and does the test have actual value ? If not, remove the test. 2. If the test has value: does faking the functions have real value ? Or would it be better for the test to be skipped when the required functionality is not available ? If faking the functions was a hack and has no real value, the `fakefunctions.php` file should be removed and the test should get `@requires extension mbstring` and `@requires function idn_to_ascii` annotations instead. 3. If the test has value and faking the functions has value too: what would be a better return value for the faked functions ? At the very least, the return type of the faked functions should be made to comply with the PHP documented return type of the functions. Having said all that and not having the answers to these questions, this commit implements a stop-gap solution to at least prevent the faked functions from influencing tests other than the test they are intended for. The stop-gap solution isolates the test to be a) run in its own process and b) run last, so as not to influence other tests. This is done by moving the test to its own TestClass, which then allows for moving the test from the "main" test suite to a separate test suite. Why this solves the issue (for now): * Test suites are run in the order they are defined (at least they should be as long as the PHPUnit `--order-by` flag is not used). * (In my experience) Test suites are each run in their own PHP process, which means that the fake functions should now only be defined for the test they were intended for. While PHPUnit also offers `@run[Tests]InSeperateProcess` annotations, in my experience, using a separate test suite is more stable for tests which need to run on a large range of PHPUnit versions, especially when we're talking functions being redefined. --- phpunit.xml.dist | 5 ++++ test/PHPMailer/AddAddressWithIDNTest.php | 33 ++++++++++++++++++++++++ test/PHPMailer/PHPMailerTest.php | 10 ------- 3 files changed, 38 insertions(+), 10 deletions(-) create mode 100644 test/PHPMailer/AddAddressWithIDNTest.php diff --git a/phpunit.xml.dist b/phpunit.xml.dist index ac8970539..e0be34e2d 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -12,6 +12,11 @@ ./test/ + test/PHPMailer/AddAddressWithIDNTest.php + + + + ./test/PHPMailer/AddAddressWithIDNTest.php diff --git a/test/PHPMailer/AddAddressWithIDNTest.php b/test/PHPMailer/AddAddressWithIDNTest.php new file mode 100644 index 000000000..1dd06fef1 --- /dev/null +++ b/test/PHPMailer/AddAddressWithIDNTest.php @@ -0,0 +1,33 @@ + + * @author Andy Prevost + * @copyright 2012 - 2020 Marcus Bointon + * @copyright 2004 - 2009 Andy Prevost + * @license https://www.gnu.org/licenses/old-licenses/lgpl-2.1.html GNU Lesser General Public License + */ + +namespace PHPMailer\Test\PHPMailer; + +use PHPMailer\PHPMailer\PHPMailer; +use PHPMailer\Test\TestCase; + +/** + * @covers \PHPMailer\PHPMailer\PHPMailer::addAddress + */ +final class AddAddressWithIDNTest extends TestCase +{ + public function testGivenIdnAddress_addAddress_returns_true() + { + if (file_exists(\PHPMAILER_INCLUDE_DIR . '/test/fakefunctions.php') === false) { + $this->markTestSkipped('/test/fakefunctions.php file not found'); + } + + include \PHPMAILER_INCLUDE_DIR . '/test/fakefunctions.php'; + $this->assertTrue($this->Mail->addAddress('test@françois.ch')); + } +} diff --git a/test/PHPMailer/PHPMailerTest.php b/test/PHPMailer/PHPMailerTest.php index 0b82fe125..dcf9d8266 100644 --- a/test/PHPMailer/PHPMailerTest.php +++ b/test/PHPMailer/PHPMailerTest.php @@ -1258,16 +1258,6 @@ public function testSmtpConnect() $this->Mail->smtpClose(); } - public function testGivenIdnAddress_addAddress_returns_true() - { - if (file_exists(\PHPMAILER_INCLUDE_DIR . '/test/fakefunctions.php') === false) { - $this->markTestSkipped('/test/fakefunctions.php file not found'); - } - - include \PHPMAILER_INCLUDE_DIR . '/test/fakefunctions.php'; - $this->assertTrue($this->Mail->addAddress('test@françois.ch')); - } - public function testErroneousAddress_addAddress_returns_false() { $this->assertFalse($this->Mail->addAddress('mehome.com'));