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

[FrameworkBundle] Add HttpClientAssertionsTrait which provide shortcuts to assert HTTP calls was triggered #50662

Merged
merged 1 commit into from Oct 2, 2023

Conversation

welcoMattic
Copy link
Member

@welcoMattic welcoMattic commented Jun 14, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR symfony/symfony-docs#18970

This PR brings a new collection (to enhance) of assertions to help users to ensure some HTTP calls has been triggered during a Request processing.

Example

An example is better than thousands words:

HttpClientAssertionsTest.php

public function testHttpClientAssertions()
{
    $client = static::createClient();
    $client->enableProfiler();

    $client->request('GET', '/foo');

    $this->assertHttpClientRequest($client, 'symfony.http_client', 'https://symfony.com/');
    $this->assertHttpClientRequest($client, 'symfony.http_client', 'https://symfony.com/', 'POST', 'foo');
    $this->assertHttpClientRequest($client, 'symfony.http_client', 'https://symfony.com/', 'POST', ['foo' => 'bar']);
    $this->assertHttpClientRequest($client, 'symfony.http_client', 'https://symfony.com/', 'POST', ['foo' => 'bar']);
    $this->assertNotHttpClientRequest($client, 'symfony.http_client', 'https://laravel.com');


    $this->assertHttpClientRequestCount($client, 'symfony.http_client', 5);
}

FooController.php

public function index(HttpClientInterface $symfonyHttpClient)
{
    $symfonyHttpClient->request('GET', '/');
    $symfonyHttpClient->request('POST', '/', ['body' => 'foo']);
    $symfonyHttpClient->request('POST', '/', ['body' => ['foo' => 'bar']]);
    $symfonyHttpClient->request('POST', '/', ['json' => ['foo' => 'bar']]);
    $symfonyHttpClient->request('GET', '/doc/current/index.html');

    return new Response();
}

http_client.yaml

framework:
    http_client:
        scoped_clients:
            symfony.http_client:
                base_uri: 'https://symfony.com'

This is a first try to improve DX in test, to allow users to assert more complex things (here it's HTTP calls, but I think also about adding assertions on logs for example).

Todo

  • Write doc

@carsonbot carsonbot added this to the 6.4 milestone Jun 14, 2023
@welcoMattic welcoMattic changed the title Add HttpClientAssertionsTrait which provide shortcut to assert HTTP calls was triggered [FrameworkBundle] Add HttpClientAssertionsTrait which provide shortcut to assert HTTP calls was triggered Jun 14, 2023
@welcoMattic welcoMattic force-pushed the profiler-asserts branch 2 times, most recently from b8c4786 to 92b819c Compare June 14, 2023 15:48
@welcoMattic welcoMattic force-pushed the profiler-asserts branch 3 times, most recently from 3a2eb65 to 1d5c59b Compare June 16, 2023 08:11
@welcoMattic
Copy link
Member Author

I've update the examples in PR description. Now we allow assertion on a single URL (with method and optional body). I've added the opposition assertion assertHttpClientHasNotBeenCalledForUrl and assertHttpClientCountRequests.

@OskarStark OskarStark changed the title [FrameworkBundle] Add HttpClientAssertionsTrait which provide shortcut to assert HTTP calls was triggered [FrameworkBundle] Add HttpClientAssertionsTrait which provide shortcut to assert HTTP calls was triggered Jun 19, 2023
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Random thoughts:

No need for checking headers using these?
Can't we remove the $client argument somehow? (don't we have it in the context somewhere?)
WHat about defaulting $httpClientId default to http_client?

@welcoMattic
Copy link
Member Author

welcoMattic commented Sep 19, 2023

@nicolas-grekas

Random thoughts:

No need for checking headers using these?

Sure, I can add header checks 👍

Can't we remove the $client argument somehow? (don't we have it in the context somewhere?)

As BrowserKitAssertionTrait::getClient is private, we don't have any other way to get the client here. Should we dig deep to find a way to do?

We have access to this trait through WebTestAssertionsTrait in WebTestCase, so we can call static::getClient()

WHat about defaulting $httpClientId default to http_client?

Nice idea

@welcoMattic welcoMattic force-pushed the profiler-asserts branch 6 times, most recently from 28948b5 to 65e5d28 Compare September 19, 2023 13:54
@welcoMattic welcoMattic changed the title [FrameworkBundle] Add HttpClientAssertionsTrait which provide shortcut to assert HTTP calls was triggered [FrameworkBundle] Add HttpClientAssertionsTrait which provide shortcuts to assert HTTP calls was triggered Sep 19, 2023
@weaverryan
Copy link
Member

Same thoughts as Nicolas: if we can remove the need for the client arg and service name (when using the default), I think I’ll really dig it!

@welcoMattic
Copy link
Member Author

@weaverryan it's done :) I've removed it https://github.com/symfony/symfony/pull/50662/files#diff-be0ff1fa989a87452ee0ecc8a932f09d50d59d3ecb4c6bce9565202aa254bd37R23.

Through traits we have access to getClient from BrowserKitAssertionsTrait, et tada!

@fabpot
Copy link
Member

fabpot commented Oct 2, 2023

Thank you @welcoMattic.

@fabpot fabpot merged commit 693276e into symfony:6.4 Oct 2, 2023
6 of 9 checks passed
@welcoMattic welcoMattic deleted the profiler-asserts branch October 2, 2023 21:41
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 4, 2023
…tation (welcoMattic)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle] Add HttpClientAssertions trait documentation

Ref symfony/symfony#50662

Commits
-------

caa3483 Add HttpClientAssertions trait documentation
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants