Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestCase should not extend Assert #5433

Open
BafS opened this issue Jun 29, 2023 · 2 comments
Open

TestCase should not extend Assert #5433

BafS opened this issue Jun 29, 2023 · 2 comments
Labels
feature/assertion Issues related to assertions and expectations type/enhancement A new idea that should be implemented

Comments

@BafS
Copy link

BafS commented Jun 29, 2023

TestCase is currently 2276 lines long and contains 129 functions, we can argue that this class concentrates too much responsibilities and is a god class. Unfortunately this class extends Assert which is 2293 lines long and contains 175 functions.

  • Any phpunit test must extends the TestCase (thus Assert) and inherit from too much logic/functions
  • It's not possible to remove assertions from userland tests
    • Personally and where I work we always use the FQCN for assertions (for example Assert::assertTrue(...)) to use composition. Unfortunately to enforce this convention you need to rely on external tools.
  • To add new assertions, people often add them is some userland abstract test classes to keep the same syntax self::assertXxx(). This bloat classes even more, I saw it many times in the wild. If Assert was not available by default it would help to promote usage of composition over inheritance and create its own "Assert".

My question is: could we remove the Assert inheritance from TestCase? What is the advantage of extending Assert? Writing self:: instead of Assert:: doesn't seem to be a good reason.

Proposal

  • Move the assertions in a trait, AssertTrait
  • Remove inheritance of Assert in TestCase but "use" AssertTrait to not have any BC in the next major
  • Promote usage of Assert::

Because there is a trait, it would be easy to have our own Assert by using use AssertTrait and add methods.

class MyAssert {
    use PhpUnitAssert;

   // custom methods...
}

In a future version we could imagine to remove the trait from TestCase. By doing so, devs would be free to either use AssertTrait to keep the "legacy" syntax or to use composition (Assert::).

class MyTest extends TestCase {
    use PhpUnitAssert;

    // We can use "self::assertXxx()"
}

or

class MyTest extends TestCase {
    // We can use "Assert::assertXxx()"
}
@BafS BafS added the type/enhancement A new idea that should be implemented label Jun 29, 2023
@sebastianbergmann
Copy link
Owner

In the long run: yes.

@sebastianbergmann sebastianbergmann added the feature/assertion Issues related to assertions and expectations label Jun 29, 2023
@thbley
Copy link

thbley commented Jul 5, 2023

Maybe offer to write Test classes without "extends TestCase".

Example:

<?php declare(strict_types=1);

use PHPUnit\Framework\Assert;
use PHPUnit\Framework\TestCaseInterface;

final class GreeterTest
{
    private static $dbh;
    
    private TestCaseInterface $testCase;

    public function __construct(TestCaseInterface $testCase) // setUp()
    {
        $this->testCase = $testCase;
    }

    public function __destruct(): void // tearDown()
    {
    }

    public static function setUpBeforeClass(): void
    {
        self::$dbh = new PDO('sqlite::memory:');
    }

    public static function tearDownAfterClass(): void
    {
        self::$dbh = null;
    }

    public function testGreetsWithName(): void
    {
        $greeter = new Greeter;

        $greeting = $greeter->greet('Alice');

        $this->testCase->expectException(Exception::class);
        
        $this->testCase->assertSame('Hello, Alice!', $greeting); // alternative 1
        Assert::assertSame('Hello, Alice!', $greeting); // alternative 2
    }
}

@sebastianbergmann sebastianbergmann changed the title TestCase should not extends Assert TestCase should not extend Assert Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

3 participants