[Testing] Restructure/rewrite testing docs #14731

wouterj · 2020-12-22T16:29:45Z

These are my first ideas for a better testing documentation. One of the major results of the Testing survey (and Testing initiative of @Nyholm, @weaverryan and @kbond) was a lack of good documentation beyond the basic stuff.

This is a very very rough sketch of my ideas. I've mostly moved sections around and added some titles. So the actual content probably no longer reads as one article, which is going to be fixed before marking this PR as finished.

Nyholm

Excellent work. Well done.

I had a few comments. You probably already know but I just wanted to share.

testing.rst

noniagriconomie · 2020-12-23T14:54:04Z

testing.rst

+``PostController`` class, start by creating a new ``PostControllerTest.php``
+file that extends a special ``WebTestCase`` class.
+
+As an example, a test could look like this::


what about referencing make:functional-test and make:unit-test from the https://github.com/symfony/maker-bundle?

can be a good bootstraping code, like for fixtures above :)

For the record, these commands are now deprecated and a new make:test command has been introduced: https://github.com/symfony/maker-bundle/blob/main/src/Maker/MakeTest.php#L38-L42

IMHO the docs should be consistent with this command, and explain all TestCase types that are available.

Yes, agreed. For now, I've updated the "Application Tests" section to use the new make:test command. Let's rework this thing in a follow-up PR.

Nyholm

Great work. Here are some minor comments.

testing.rst

dunglas · 2021-03-09T11:11:04Z

testing.rst


 .. code-block:: terminal

-    $ composer require --dev symfony/css-selector
+    $ composer require --dev dama/doctrine-test-bundle


Shouldn't we add this package to test-pack then?

I've been trying to recommend dama/doctrine-test-bundle as a performance tool rather than a reset tool. Transactions can be daunting to newcomers and can make debugging a failing test difficult (the data isn't in your test database after a test fails).

I think the plan is to add zenstruck/foundry to the test-pack and recommend using it's ResetDatabase trait (which doesn't, by default, rely on transactions to reset).

(this comment is a response to #14731 (comment))

I agree. However, as we know, there is no official reset tool for Doctrine, nor is there a library that only does the reset logic. So we'll have to wait for Foundry to be documented in this article before we can recommend the ResetDatabase trait for resetting and only use the dama bundle for performance.

Also, this PR targets 4.4 (to avoid merge conflicts for the next years). We'll probably only want to talk about Foundry in the 5.3+ version of the docs.

dunglas · 2021-03-09T11:13:03Z

testing.rst


-.. versionadded:: 4.3
+That's it! This bundle uses a clever trick: it begins a database


It will probably break tests using Panther. Can this be disabled on a per-test basis?

I don't believe so.

Can this be disabled on a per-test basis?

Theoretically its possible. But this would defeat the purpose a bit since then some tests will actually persist changes in the DB and potentially impact other tests.

In this case I think one should split the tests into different suites and only enable dama/doctrine-test-bundle for non-Panther tests maybe.

dunglas · 2021-03-09T11:16:13Z

testing.rst

+``PostController`` class, start by creating a new ``PostControllerTest.php``
+file that extends a special ``WebTestCase`` class.
+
+As an example, a test could look like this::


For the record, these commands are now deprecated and a new make:test command has been introduced: https://github.com/symfony/maker-bundle/blob/main/src/Maker/MakeTest.php#L38-L42

IMHO the docs should be consistent with this command, and explain all TestCase types that are available.

dunglas · 2021-03-09T11:19:37Z

testing.rst

-
-.. index::
-   pair: PHPUnit; Configuration
+TODO


Could we also add a section about ApiTestCase (or a link to this page)?

Yes, let's do that in a follow-up PR :)

Nyholm · 2021-03-09T11:25:21Z

Awesome. Thank you Kevin!
I agree with all your comments. @wouterj I'll let you decide what actions to take on Kevin's review.

My suggestion is that we should try to wrap up this PR to get it merged (hopefully this week) and I'll be happy to take actions on the comments in a separate PR.

…Nyholm) This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [FrameworkBundle] Add KernelTestCase::getContainer() | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | | License | MIT | Doc PR | symfony/symfony-docs#14731 There are at least 3 ways to get the container in a test class: ```php class FooTest extends WebTestCase { public function testGetContainerA() { $kernel = self::bootKernel(); $container = $kernel->getContainer(); } public function testGetContainerB() { self::bootKernel(); $container = self::$container; } public function testGetContainerC() { $client = self::createClient(); $container = $client->getContainer(); } } ``` I suggest to add a fourth =) Basically, in tests you should always use the `test.service_container`. It is hard to remove A and C, but I can deprecate C and add a helper function. ```php class FooTest extends WebTestCase { public function testGetContainerTheOnlyWayYouShouldUse() { $container = $this->getContainer(); } } ``` This new way will also boot your kernel if it is not already booted. Commits ------- f4c9724 [FrameworkBundle] Add KernelTestCase::getContainer()

…Nyholm) This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [FrameworkBundle] Add KernelTestCase::getContainer() | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | | License | MIT | Doc PR | symfony/symfony-docs#14731 There are at least 3 ways to get the container in a test class: ```php class FooTest extends WebTestCase { public function testGetContainerA() { $kernel = self::bootKernel(); $container = $kernel->getContainer(); } public function testGetContainerB() { self::bootKernel(); $container = self::$container; } public function testGetContainerC() { $client = self::createClient(); $container = $client->getContainer(); } } ``` I suggest to add a fourth =) Basically, in tests you should always use the `test.service_container`. It is hard to remove A and C, but I can deprecate C and add a helper function. ```php class FooTest extends WebTestCase { public function testGetContainerTheOnlyWayYouShouldUse() { $container = $this->getContainer(); } } ``` This new way will also boot your kernel if it is not already booted. Commits ------- f4c97240ff [FrameworkBundle] Add KernelTestCase::getContainer()

OskarStark · 2021-03-12T07:00:30Z

@dmaicher maybe you want to have look at his PR 👀

testing.rst

kbond

Looks good! Just a few minor comments.

testing.rst

kbond · 2021-04-15T12:56:53Z

testing.rst


 .. code-block:: terminal

-    $ composer require --dev symfony/css-selector
+    $ composer require --dev dama/doctrine-test-bundle


I've been trying to recommend dama/doctrine-test-bundle as a performance tool rather than a reset tool. Transactions can be daunting to newcomers and can make debugging a failing test difficult (the data isn't in your test database after a test fails).

I think the plan is to add zenstruck/foundry to the test-pack and recommend using it's ResetDatabase trait (which doesn't, by default, rely on transactions to reset).

(this comment is a response to #14731 (comment))

kbond · 2021-04-15T13:02:29Z

testing.rst

-    namespace App\Tests\Controller;
+If you need to customize some environment variables for your tests (e.g. the
+``DATABASE_URL`` used by Doctrine), you can do that by overriding anything you
+need in your ``.env.test`` file:


Environment variables can also be added to your phpunit.xml. When would you use phpunit.xml over .env.test? Is there a reason these two env variables can't be added to .env.test?

I'm not sure, I think they can be part of .env.test. @nicolas-grekas do you maybe know why they aren't?

* Moved around things * Don't mention PHPUnitBridge * Add info about integration tests

wouterj · 2021-04-17T13:06:23Z

I think this PR is now ready to be merged. The rewrite is not fully finished, but finished enough to be merged imho. Merging this now allows Tobias and others to also work on the document.

Among some other things, my major open points with this article are: move most of the dom crawler section to a subarticle and favor the Symfony assertion traits; use make:test in all sections; document the other options of make:test.

@javiereguiluz or @weaverryan do you mind having a look at this PR (and maybe merge it)?

javiereguiluz

This was a fantastic work! Thanks a lot for the effort put in this task.

testing.rst

wouterj · 2021-04-17T15:48:11Z

Aaand it's merged. Thanks for your blazing fast response on this Saturday afternoon @javiereguiluz!

And a special big thanks to @Nyholm for doing the bulk of the work (including pursuing me to finally finish this PR).

Nyholm · 2021-04-17T16:14:35Z

What?! This is awesome. Thank you all for the help with this <3

Nyholm reviewed Dec 22, 2020

View reviewed changes

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

kbond reviewed Dec 22, 2020

View reviewed changes

testing.rst Outdated Show resolved Hide resolved

noniagriconomie reviewed Dec 23, 2020

View reviewed changes

wouterj mentioned this pull request Jan 21, 2021

[Security][Testing] Moved a performance tip to the main performance article #14864

Closed

wouterj mentioned this pull request Feb 4, 2021

Add a new make:test command symfony/maker-bundle#807

Merged

wouterj mentioned this pull request Feb 27, 2021

Starting recommending to run PHPUnit directly symfony/recipes#906

Merged

Nyholm mentioned this pull request Mar 4, 2021

[FrameworkBundle] Add KernelTestCase::getContainer() symfony/symfony#40366

Merged

kbond mentioned this pull request Mar 5, 2021

Add zenstruck/foundry symfony/recipes#908

Merged

wouterj force-pushed the testing branch from 3428726 to dc46dd0 Compare March 7, 2021 22:36

wouterj changed the title ~~[Very WIP][Testing] Restructure/rewrite testing docs~~ [WIP][Testing] Restructure/rewrite testing docs Mar 7, 2021

wouterj force-pushed the testing branch from dc46dd0 to bb115f7 Compare March 7, 2021 22:42

Nyholm reviewed Mar 8, 2021

View reviewed changes

testing.rst Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

dunglas reviewed Mar 9, 2021

View reviewed changes

wouterj mentioned this pull request Mar 9, 2021

[POC] add ResetDatabase trait for resetting database between tests doctrine/DoctrineBundle#1272

Closed

OskarStark reviewed Mar 12, 2021

View reviewed changes

testing.rst Outdated Show resolved Hide resolved

kbond reviewed Apr 15, 2021

View reviewed changes

wouterj and others added 2 commits April 17, 2021 13:10

Very rough sketch of better testing docs

1380bd7

Finalize testing document rewrite

42cb887

* Moved around things * Don't mention PHPUnitBridge * Add info about integration tests

wouterj force-pushed the testing branch 3 times, most recently from 78d3e42 to e114598 Compare April 17, 2021 13:04

wouterj marked this pull request as ready for review April 17, 2021 13:04

carsonbot added Status: Needs Work Testing labels Apr 17, 2021

carsonbot added this to the 4.4 milestone Apr 17, 2021

carsonbot changed the title ~~[WIP][Testing] Restructure/rewrite testing docs~~ [Testing] [WIP] Restructure/rewrite testing docs Apr 17, 2021

wouterj changed the title ~~[Testing] [WIP] Restructure/rewrite testing docs~~ [Testing] Restructure/rewrite testing docs Apr 17, 2021

wouterj force-pushed the testing branch from e114598 to e02b521 Compare April 17, 2021 13:07

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 17, 2021

wouterj force-pushed the testing branch from e02b521 to cfd2408 Compare April 17, 2021 13:09

javiereguiluz approved these changes Apr 17, 2021

View reviewed changes

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

testing.rst Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 17, 2021

wouterj force-pushed the testing branch from cfd2408 to 90188e7 Compare April 17, 2021 15:43

Finish testing article rewrites

1091d13

wouterj force-pushed the testing branch from 90188e7 to 1091d13 Compare April 17, 2021 15:46

wouterj merged commit 6d99029 into symfony:4.4 Apr 17, 2021

wouterj deleted the testing branch April 17, 2021 15:48

javiereguiluz mentioned this pull request Apr 21, 2021

Update testing.rst #15183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Testing] Restructure/rewrite testing docs #14731

[Testing] Restructure/rewrite testing docs #14731

wouterj commented Dec 22, 2020

Nyholm left a comment

noniagriconomie Dec 23, 2020

dunglas Mar 9, 2021

wouterj Apr 17, 2021

Nyholm left a comment

dunglas Mar 9, 2021

kbond Apr 15, 2021 •

edited

Loading

wouterj Apr 17, 2021

dunglas Mar 9, 2021

kbond Mar 9, 2021

dmaicher Mar 12, 2021

dunglas Mar 9, 2021

dunglas Mar 9, 2021

wouterj Apr 17, 2021

Nyholm commented Mar 9, 2021

OskarStark commented Mar 12, 2021

kbond left a comment

kbond Apr 15, 2021 •

edited

Loading

kbond Apr 15, 2021

wouterj Apr 17, 2021

wouterj commented Apr 17, 2021 •

edited

Loading

javiereguiluz left a comment

wouterj commented Apr 17, 2021

Nyholm commented Apr 17, 2021


		.. versionadded:: 4.3
		That's it! This bundle uses a clever trick: it begins a database

[Testing] Restructure/rewrite testing docs #14731

[Testing] Restructure/rewrite testing docs #14731

Conversation

wouterj commented Dec 22, 2020

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbond Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 9, 2021

OskarStark commented Mar 12, 2021

kbond left a comment

Choose a reason for hiding this comment

kbond Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Apr 17, 2021 • edited Loading

javiereguiluz left a comment

Choose a reason for hiding this comment

wouterj commented Apr 17, 2021

Nyholm commented Apr 17, 2021

kbond Apr 15, 2021 •

edited

Loading

kbond Apr 15, 2021 •

edited

Loading

wouterj commented Apr 17, 2021 •

edited

Loading