symfony / panther

A browser testing and web crawling library for PHP and Symfony
MIT License
2.94k stars 222 forks source link

Impossible to get the HttpFoundation objects from the Client #17

Closed Pierstoval closed 6 years ago

Pierstoval commented 6 years ago
Q A
Bug report? yes
Feature request? no
BC Break report? yes

When using PanthereTestCase, and when making a request, we can't get access to the Symfony\Component\HttpFoundation\Response object that should normally be sent to the Kernel. Instead, we only get access to the BrowserKit one.

Steps to reproduce:

<?php

use Panthere\PanthereTestCase;

class MyPanthereTest extends PanthereTestCase
{
    public function testMyApp()
    {
        $client = static::createPanthereClient();
        $crawler = $client->request('GET', static::$baseUri.'/any-page');

        static::assertSame(200, $client->getResponse()->getStatusCode());
    }
}

PHPUnit's output:

Error : Call to undefined method Symfony\Component\BrowserKit\Response::getStatusCode()

I consider this as a BC break, because the $client->request() method SHOULD return an instance of the Symfony\Component\BrowserKit\Client class, for which getResponse() returns an instance of Symfony\Component\HttpFoundation\Response.

This is the same for the Request via $client->getRequest() by the way.

The issue comes from these lines: https://github.com/dunglas/panthere/blob/b3f0601e105010d365360f238ca1b572a4e5ec82/src/Client.php#L238-L241

IMO, the Request and Response object should be either blocked from the PanthereClient via an exception (easy pick), or created & adapted based on the WebDriver results (harder).

WDYT?

dunglas commented 6 years ago

I’m for blocking them, because in some cases we cannot access to the exact HTTP request issued by the browser.

Pierstoval commented 6 years ago

I’m for blocking them, because in some cases we cannot access to the exact HTTP request issued by the browser.

Even though we could still create a Response object based on the response headers retrieved from the client? Isn't there an API to communicate with the browser and retrieve request & response data? This would be great help in creating BC Request and Response objects..

dunglas commented 6 years ago

It’s not supported by the WebDriver protocol: https://stackoverflow.com/questions/6509628/how-to-get-http-response-code-using-selenium-webdriver-with-java

Pierstoval commented 6 years ago

Too bad, then let's go for a blocker πŸ˜•

stof commented 6 years ago

the BrowserKit component does not require at all that you can get the HttpFoundation objects from it. Client::getResponse actually has a return type differing based on the implementation (due to a mistake in the HttpKernel component implementation early on, which was identified only years later and so could not be broken). So you have no guarantee on the available methods (return type is object|null). Use Client::getInternalResponse to always get a BrowserKit Response object in all implementations.

then, the fact that the status code will not be accurate (always 200 due to webdriver not exposing it) is a separate issue.

dunglas commented 6 years ago

Can we close this issue?

Pierstoval commented 6 years ago

I think we should add a blocker in the API that sends an exception to remind that the request is not available because of the driver πŸ€”

stof commented 6 years ago

@Pierstoval in which API ? The method typed as @return object is returning a different object for which the method does not exist

Pierstoval commented 6 years ago

In Panthere's one, check #42, could you give feedback to know whether it's a correct way to do it or not?

d42ohpaz commented 4 years ago

Hi, I know this is closed, but why wasn't the decision made to proxy Client::getInternalResponse() through Client::getReponse()? Throwing an exception means that it breaks compatibility with the BrowserKitAssertionsTrait via the WebTestAssertionsTrait; i.e., calling self::assertResponseIsSuccessful() will throw an exception because it relies on Client::getResponse().

I'm new to using Panther, and I'm trying to understand the decision process so I can write better tests. Thank you!

gcollombet commented 3 years ago

Hi, I know this is closed, but why wasn't the decision made to proxy Client::getInternalResponse() through Client::getReponse()? Throwing an exception means that it breaks compatibility with the BrowserKitAssertionsTrait via the WebTestAssertionsTrait; i.e., calling self::assertResponseIsSuccessful() will throw an exception because it relies on Client::getResponse().

I'm new to using Panther, and I'm trying to understand the decision process so I can write better tests. Thank you!

Same problem for me

stof commented 3 years ago

@9ae8sdf76 the issue is that Webdriver does not handle a HttpFoundation response at all, so it cannot return it.

regarding WebTestAssertionsTrait, the right fix would be to change the trait to use Client::getInternalResponse() whenever it can, which is guaranteed to return a browserkit response for all implementations.

Pierstoval commented 3 years ago

If you want an HttpFoundation Response object, the only way (for now) is to create it yourself with everything that's available through the browserkit response and/or what the Webdriver returns

dunglas commented 3 years ago

@Pierstoval this will be almost useless because - by design - WebDriver doesn't expose the HTTP response and there is no way to access to the headers or to the status code.

Pierstoval commented 3 years ago

Panther is for E2E tests anyway, so if one needs to check the returned HTTP response instead of the DOM page, they'd better use a simpler WebTestCase-based setup, it's more performant, and does what's needed for that πŸ˜…

snoweuph commented 2 months ago

This means that self::assertRouteSame('test') is also broken, so you would have to instead write $this->assertStringEndsWith("/test", $client->getCurrentURL()); or $this->assertStringContainsString("/test", $client->getCurrentURL()); couldn't Panther just overwrite the assertRouteSame method and some other useful defaults of symfony directly?

Pierstoval commented 2 months ago

@snoweuph That's a lot of things that Panther would need to do after a Request. It's not that it's impossible, but rather that it's really hard.

Think about it: how would you know what route you called if you just had your HTTP server and Curl to make the check?

The whole point of Panther (as well as any e2e-related tool) is that you make true HTTP requests. As I said in an earlier comment, if you "just" want the name of the route, you have no need of Panther for this test, and you could replace it with a WebTestCase instead, because then it will call the Kernel, and you will have access to the Request object too, the one containing the attributes with _route and _route_params for you to assert on πŸ‘

Panther is mostly for browser-based testing, especially testing that your Javascript code works as expected. If you're doing it for "static HTML content" or "Symfony-Kernel-related data", you're not using it for what it was made for.

snoweuph commented 2 months ago

@snoweuph That's a lot of things that Panther would need to do after a Request. It's not that it's impossible, but rather that it's really hard.

Think about it: how would you know what route you called if you just had your HTTP server and Curl to make the check?

The whole point of Panther (as well as any e2e-related tool) is that you make true HTTP requests. As I said in an earlier comment, if you "just" want the name of the route, you have no need of Panther for this test, and you could replace it with a WebTestCase instead, because then it will call the Kernel, and you will have access to the Request object too, the one containing the attributes with _route and _route_params for you to assert on πŸ‘

Panther is mostly for browser-based testing, especially testing that your Javascript code works as expected. If you're doing it for "static HTML content" or "Symfony-Kernel-related data", you're not using it for what it was made for.

In my case the tests are about following specific clicking routes user should follow, and to make the test fail early the routes in between should be checked (the end page the other hand might not even be on the symfony applicaiton itselve)

The whole Idea here is to do the actually clicking, to test the real connection between ages, instead of just sending speicifc site requests, and staying on them

Pierstoval commented 2 months ago

In my case the tests are about following specific clicking routes user should follow

@snoweuph In your case, you can definitely "click a link" without using Panther, because you have access to the DOM (though it's not compatible with JS), and you can fetch an anchor, create a Link object with, and ask the Client to "click" on it. This way, you can check the routes after each click.

If you don't need JS, this is the way to go: https://symfony.com/doc/current/components/dom_crawler.html#links

snoweuph commented 2 months ago

In my case the tests are about following specific clicking routes user should follow

@snoweuph In your case, you can definitely "click a link" without using Panther, because you have access to the DOM (though it's not compatible with JS), and you can fetch an anchor, create a Link object with, and ask the Client to "click" on it. This way, you can check the routes after each click.

If you don't need JS, this is the way to go: https://symfony.com/doc/current/components/dom_crawler.html#links

I was just saying that some builtins don't work, even though in the readme it's saying that you can just use all the built-in asserts. And I've already stated how I'm currently doing it because those don't work, to get the same behavior.

I don't know the Project Philosophy, but having to guess, it would either make sense to override all builtins to work, or to at least state which ones don't work somewhere in the readme/doc

Pierstoval commented 2 months ago

Maybe throwing errors explaining the incompatibility, right in the PantherTestCase might be indeed logical to understand why it's impossible to assert based on an HttpFoundation Response or Request object. Feel free to provide a PR if you can, that would certainly be helpful πŸ‘