Skip to content

Commit

Permalink
Tests: isolate test which fakes functions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jrfnl committed Sep 11, 2024
1 parent aaddfdf commit 98e110e
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 10 deletions.
5 changes: 5 additions & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
<testsuites>
<testsuite name="PHPMailerTests">
<directory>./test/</directory>
<exclude>test/PHPMailer/AddAddressWithIDNTest.php</exclude>
</testsuite>
<!-- Separate group for tests which include the `fakefunctions.php` file as it can screw up other tests. -->
<testsuite name="PHPMailerWithFakedFunctionsTests">
<file>./test/PHPMailer/AddAddressWithIDNTest.php</file>
</testsuite>
</testsuites>
<listeners>
Expand Down
33 changes: 33 additions & 0 deletions test/PHPMailer/AddAddressWithIDNTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

/**
* PHPMailer - PHP email transport unit tests.
* PHP version 5.5.
*
* @author Marcus Bointon <phpmailer@synchromedia.co.uk>
* @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'));
}
}
10 changes: 0 additions & 10 deletions test/PHPMailer/PHPMailerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Expand Down

0 comments on commit 98e110e

Please sign in to comment.