Injecting dependencies and using Phpunit mocks after 3.2 deprecation #23311
Comments
|
You can use |
|
This is a documentation issue, we need to give the right way to use specific services for functional testing. I opened symfony/symfony-docs#8203, closing here. |
|
Could this please be reopened? My comment on #21533:
If it just isn't possible to restore this functionality, we need a solution for testing that is more robust than "use an alternate config file". |
|
I would like a reopen too, if possible. Any feedback on my proposed feature would be appreciated. |
|
@emil-nasso see #23772. If you have a different use case that you think cannot be solved with the solution proposed in the closed issue, please consider opening a new one. |
|
@chalasr Sure, I probably could. In my case, there is a mix of tests that should use the mocked client and old tests that should use the production client. This issue is all about DX, it's not about if it can be solved with other solutions or not. Our project has tons of tests and many many of them use set on the container. Before this deprecation, for us, Symfony was: "Nice, Symfony is very easy to test". Now, it's more along the lines of "There is a huge headache and we have to rewrite TONS of tests". There is not an easy and clear replacement for or migration path away from This feature would provide that. This depreciation, even though I understand why it was deprecated, is a clear step back in DX. |
|
Thinking about this twice, we should not revert that 3.2 deprecation: setting a private service should nowhere be supported. We could remove the 3.3 deprecation that also discourages setting predeclared-and-public services. BUT, that wouldn't solve the DX issue reported here, because I guess you (or someone else) will want to mock both some public or private services at some point. There is another way to make your tests work again without refactoring everything: if you make the services you want to mock as public and synthetic, then you will be allowed to set them. Doing so still requires some configuration specific for tests, but then, there is no need to change the test cases. That's another way we could document, isn't it? |
|
@nicolas-grekas I will look into synthetic/public services, was not aware of that. I created an example of how you could do this kind of testing in lumen, just to illustrate the DX issue. I have an external dependency. It's a service that builds cars. How it is implemented does not really matter here. Its public contract does, however. This could just as well be an external service accessed over php, some third-party client, a database connection or anything else that is out of scope to test for this application. We want to mock this. class CarService
{
private $cars = [];
/**
* Will return a list of all current cars.
*
* Example:
*
* list() =>
* [
* ["brand" => "imagination brand", "color" => "blue"],
* ["brand" => "toyota", "color" => "purple"]
* ]
* @return array
*/
function list() { ... }
/**
* Build a car.
* Will throw Exception when something goes wrong and log it.
* @param $brand
* @param $color
* @throws Exception
*/
function build($brand, $color) { ... }
}The controller that uses this service looks as below. This is the code we actually want to test. We want to test that the re-ordering of the list of cars work as expected. We also want to test that the build method and its validation works. We also want to test that we fail gracefully if the backend (the service) has some kind of unexpected error. namespace App\Http\Controllers;
use Illuminate\Http\Request;
class CarController extends Controller
{
/**
* @var \CarService
*/
private $carService;
/**
* @param \CarService $carService
*/
public function __construct(\CarService $carService)
{
$this->carService = $carService;
}
public function list()
{
return collect($this->carService->list())->groupBy("brand")->map(function ($brand) {
return collect($brand)->map(function ($item) {
return $item["color"];
})->toArray();
})->toArray();
}
/**
* @param Request $request
* @return array
*/
public function build(Request $request)
{
$this->validate($request, [
"brand" => "required",
"color" => "required|in:green,blue",
]);
$brand = $request->get("brand");
$color = $request->get("color");
try {
$this->carService->build($brand, $color);
} catch (\Exception $e){
return ["error" => "Unknown backend error. Please contact support"];
}
}
}This is the test that tests all of this this. Each test mocks the service in a way that is appropriate for that test. Each test is very short and staightforward. We only test what we want to test, the code in the controller and everything in the application up to it (authentication, middlewares etc). We don't want to test the service. It is an external dependency and tested elsewhere. This is not end-to-end tests that should test all the way to production instances of real services or real databases. We are using PhpUnits mocks to simulate the service and assert that the expected methods gets called with the expected data the expected amount of times. class ExampleTest extends TestCase
{
public function testListByBrand()
{
$carService = $this->createMock(CarService::class);
$carService->expects($this->once())->method("list")->willReturn([
["brand" => "toyota", "color" => "pink"],
["brand" => "volvo", "color" => "white"],
["brand" => "toyota", "color" => "blue"],
]);
$this->app->instance(CarService::class, $carService);
$this->get('/cars/list-by-brand')->seeJson(["toyota" => ["pink", "blue"], "volvo" => ["white"]]);
}
public function testBuild()
{
$carService = $this->createMock(CarService::class);
$carService->expects($this->once())->method("build")->with("foo", "blue");
$this->app->instance(CarService::class, $carService);
$this->post("/cars/build", ["brand" => "foo", "color" => "blue"])->assertResponseOk();
}
public function testBuildFailsOnInvalidColor()
{
$carService = $this->createMock(CarService::class);
$carService->expects($this->never())->method("build");
$this->app->instance(CarService::class, $carService);
$this->post("/cars/build", ["brand" => "imagination brand", "color" => "pink"])->seeJson(["color" => ["The selected color is invalid."]]);
}
public function testBuildHandlesBackendErrorGracefully()
{
$carService = $this->createMock(CarService::class);
<
8000
span class="pl-c1">$carService->expects($this->once())->method("build")->willThrowException(new Exception("Something went wrong!"));
$this->app->instance(CarService::class, $carService);
$this->post("/cars/build", ["brand" => "toyota", "color" => "blue"])->seeJson(["error" => "Unknown backend error. Please contact support"]);
}
}I think this kind of testing is very nice to write, simple and straightforward for the developer. We test what we want to test and nothing else. This kind of testing was possible in symfony before the deprecation. I have not found any real alternatives in symfony yet that makes this possible. I don't want to have to fiddle around with multiple configuration files or try to cheat the container in any way by injecting something that is basically a container into the container so that I can replace instances. This example was simple, straightforward and fast to write. I want to be able to do that in Symfony too. :) Imagine having hundreds of these tests. Imagine upgrading symfony and seeing ALL those tests fail because of a deprecation message. I hope that helps with understanding why i created this issue. Is it possible to open this feature request again? Or is something in it invalid? Edit: Looked into synthetic services. That is probably the best workaround I have found so far. I could have all my services be synthetic, set them when bootstrapping the application and override them in the tests. Being able to change the service configuration before the container is started would be way better but this might be a way for us to be able to upgrade our application to a newer version of symfony. |
|
See #24418 |
…colas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [DI] Allow setting any public non-initialized services | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23311, #23772 | License | MIT | Doc PR | - I think we went a but too far with this deprecation and that we should relax the rule: setting a pre-defined service is OK *if* it has not already been initialized. This will allow setting them in test cases, as reported several times (see linked issues). Commits ------- d314b1f [DI] Allow setting any public non-initialized services
|
Very nice. Thank you! |
|
@nicolas-grekas, I know this issue is old and closed, but to you have thoughts on #27037, as it related to this issue? |
With 3.2 this kind of approach to functional testing and mocking was deprecated:
https://stackoverflow.com/questions/19726281/how-to-mock-symfony-2-service-in-a-functional-test
There is currently no clear replacement for this that doesn't require a lot of rewrites.
Take this example:
You have an application that lists products. It consists of a controller, a product repository, and a database connection. The repository uses the database connection to list products.
We would like to write some functional tests that test two scenarios: there are no products and there are products. We already have unit tests for our repository with a mocked database connection/test-database. We need to change the behavior of the repository on a per-test basis.
We have two options, as I see it, unit testing the controller and replacing the class in the test environments config file.
If we have all our controllers registered as services, inject all dependencies into them and don't access the container from the controller we can simply create an instance of the controller with phpunit mocked dependencies and unit test it. This is not a fully functional test and symfony is never booted. No events will be run, for example.
The other option is to create a RepositoryMock class that extends the Repository or implements a common interface and has a
setData()method and overrides the other methods in the original class. With this approach we can use$client = self::createClient();and$client->getContainer()->get('repository')->setData(...)in the test before making the request. This takes some work as you have to manually mock all classes and figure out a way to change their behavior with a call to public methods. We can't use phpunit mocks here as it is not possible to replace the entire object in the container.There might be a third option, that is what I'm trying to get to here. :)
The issue with the method that was deprecated is the at that stage in the execution there is no guarantee that the object has not already been used. It would be nice if you could replace objects in the container configuration when creating the container before any of the objects have been used.
Take a look at this example:
The
createClientmethod takes an options array. We pass in the services we want to override to it so that it can be passed along when booting the kernel and can be loaded when the container is created.Would this be possible? Are there any issues with this that I'm overlooking? I think this approach could potentially make testing easier and cleaner. :)
The text was updated successfully, but these errors were encountered: