NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[FrameworkBundle] Move test cases into reusable traits for ease of supporting other testing frameworks#60612

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

codetetic wants to merge1 commit intosymfony:7.4fromcodetetic:feature/improve-test-case

Closed

[FrameworkBundle] Move test cases into reusable traits for ease of supporting other testing frameworks#60612

codetetic wants to merge1 commit intosymfony:7.4fromcodetetic:feature/improve-test-case

Conversation

Copy link

codetetic commentedJun 1, 2025

Q	A
Branch?	7.4
Bug fix?	no
New feature?	yes
Deprecations?	yes
Issues	n/a
License	MIT

Changes:

Divided the helper and assertion test case logic into reusable traits.
Created prefixed public methods for easy access to test data.
Ensured a static type hint forAbstractBrowser for the$client attribute inWebTestCase.

The above will allow:

Developers to create new test cases with only the supporting features from Symfony that they need.
Ease of integrations with other testing frameworks likePest:

<?phpuse function Pest\Symfony\Web\createClient;use function Pest\Symfony\Web\getRequest;use function Pest\Symfony\Web\getResponse;it('can get a 200 response from /example', function (): void {    createClient()->request('GET', '/example');    expect(getRequest()->getMethod())->toBe('GET');    expect(getResponse()->getContent())->toMatchSnapshot();});

For more examples:https://github.com/codetetic/pest-plugin-symfony.

Upgrade notes (for the *-7.4.md files):

 * Deprecate BrowserKitAssertionsTrait for Traits\BrowserKitTrait and Traits\BrowserKitAssertionsTrait. * Deprecate private BrowserKitAssertionsTrait::getRequest() for public Traits\BrowserKitTrait::getClientRequest(). * Deprecate private BrowserKitAssertionsTrait::getResponse() for public Traits\BrowserKitTrait::getClientResponse(). * Deprecate DomCrawlerAssertionsTrait for Traits\DomCrawlerTrait and Traits\DomCrawlerAssertionsTrait. * Deprecate private DomCrawlerAssertionsTrait::getCrawler() for public Traits\DomCrawlerTrait::getClientCrawler(). * Deprecate HttpClientAssertionsTrait for Traits\HttpClientTrait and Traits\HttpClientAssertionsTrait. * new public Traits\HttpClientTrait::getHttpClientDataCollector(): Symfony\Component\HttpClient\DataCollector\HttpClientDataCollector. * Deprecate MailerAssertionsTrait for Traits\MailerTrait and Traits\MailerAssertionsTrait. * Deprecate private MailerAssertionsTrait::getMailerMessageEvents() for public Traits\MailerTrait::getMailerMessageEvents(). * Deprecate NotificationAssertionsTrait for Traits\NotifierTrait and Traits\NotifierAssertionsTrait. * Deprecate public NotificationAssertionsTrait::getNotificationEvents() for public Traits\NotifierTrait::getNotifierNotificationEvents().

carsonbot added Status: Needs Review Deprecation Feature FrameworkBundle labels

Jun 1, 2025

carsonbot added this to the7.4 milestone

Jun 1, 2025

codetetic requested a review fromyceruto as acode owner

June 1, 2025 13:13

xabbuh reviewed

Jun 1, 2025

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Test/Traits/BrowserKitAssertionsTrait.php

		*
		* @see https://laravel.com/docs/5.7/dusk#available-assertions
		*/
		trait BrowserKitAssertionsTrait

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I do not understand why we need to introduce new traits that look basically the same as the ones that are deprecated here.

Copy link

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

They are the same files but renamed with a new namespace.

The goal is to make test cases more compositable (Pest, for example, does not need the assertion methods), while maintaining full backwards compatibility.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Moving the traits is a no go from my side as that puts a lot of burden on application developers without providing any benefits if I don’t miss anything substantial.

Copy link

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you give an example of the burden?

AsKernelTestCase andWebTestCase "contracts" to the end users remain unchanged.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Everyone using the traits themselves would have to update their code.

Copy link

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Everyone using the traits themselves would have to update their code.

The orignal traits still exist and work though? They would only be removed in version 8.0, at the earliest.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sure, we don’t introduce such BC breaks in minor versions. But at some point this would still require update work and I fail to see the benefit apart from removing traits just for the sake of it.

Note that I am not against introducing new traits or adding new methods in traits if that improves reusability though.

Copy link

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

OK, would you accept the following then:

No "Traits" namespace.
Keep all existing traits and ensure BC.
Create in the base folder the following new traits:

BrowserKitTrait
DomCrawlerTrait
HttpClientTrait
MailerTrait
NotifierTrait
WebTrait
KernelTrait

These new files would contain only the helper the methods like "getClientRequest".

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The goal would be that someone not using PHPUnit would use one of the new trait and their helper methods but would never use the assertions? If that's the case, I would be 👎 on making any change at all. I have checked a few of those helper methods and none of them do not look complex enough for me to justify introducing new traits (that could potentially lead to confusion). For example, there's nothing wrong IMO in having to writeself::getClient()->getCrawler() instead ofself::getCrawler().

Copy link

Member

nicolas-grekasJun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I share@xabbuh's concerns and recommendation.
If you wanna give your proposal more time, the direction you're drawing could work. Not annoying ppl using phpunit with needless deprecations is a must to me also. Then, about the new helpers, that's yours to convince us :)

move test cases into reusable traits

f61fbcc

codetetic force-pushed thefeature/improve-test-case branch frombac07f8 tof61fbccCompare

June 1, 2025 15:55

symfony deleted a comment fromcarsonbot

Jun 2, 2025

Copy link

Member

nicolas-grekas commentedJun 2, 2025

Let me close this PR. Of course a new one with another approach is always possible.

nicolas-grekas closed this

Jun 2, 2025

Copy link

Author

codetetic commentedJun 3, 2025

Let me close this PR. Of course a new one with another approach is always possible.

Makes sense.

I'll rethink and come back with a much smaller change.

Thanks for looking at this PR.

Copy link

Author

codetetic commentedJun 3, 2025•
edited
Loading

For more context:

The problem I'm trying to solve is reconciling Symfony's subject-less based assertions:

$this->assertResponseHasCookie('name');

With Pest's subject-based assertions:

expect($response)->toHaveCookie('name');

Ideally, both projects should be able to share test helpers and constraints.

Pest is able to do this as it is built on top of PHPUnit.

The goal of my project, then is to be a very small compatibility layer.

As an example ofassertResponseHasCookie vstoHaveCookie, the code looks like:

PHPUnit:

public static function assertResponseHasCookie(string $name, string $path = '/', ?string $domain = null, string $message = ''): void{    self::assertThatForResponse(new ResponseConstraint\ResponseHasCookie($name, $path, $domain), $message);}

Pest:

$expect->extend('toHaveCookie', function (string $name, string $path = '/', ?string $domain = null): HigherOrderTapProxy|TestCall {    expect(unwrap($this->value))        ->toMatchConstraint(new ResponseConstraint\ResponseHasCookie($name, $path, $domain));    return test();});

Theunwrap function allows for a more Symfony-style assertion of the test object:

expect($this)->toHaveCookie('name');

So I'll continue with the pure Pest and Symfony style and maybe use reflection or remove the midway style:

<?phpuse function Pest\Symfony\Web\createClient;it('can get a 200 response from /example', function (): void {    $response = createClient()->request('GET', '/example');    // Pest style    expect($response)->toHaveCookie('name');    // Midway style - reflection? remove?    // expect($this)->toHaveCookie('name');    // Symfony style    $this->assertResponseHasCookie('name');});